Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Wallet.estimateJoinStakePoolFee #1094

Closed
Anviking opened this issue Nov 29, 2019 · 4 comments
Closed

Add Wallet.estimateJoinStakePoolFee #1094

Anviking opened this issue Nov 29, 2019 · 4 comments
Assignees

Comments

@Anviking
Copy link
Member

Anviking commented Nov 29, 2019

Context

We currently have selectCoinsForPayment, estimatePaymentFee and selectCoinsForDelegation.

We can estimate the fees of a normal transaction before submitting it. With WB-32 we want to estimate the fees for joining a stake-pool.

These functions do not appear to have direct unit tests.

Decision

Add a Wallet.estimateJoinStakePoolFee function to complete the pattern of the three mentioned functions.

Acceptance Criteria

  1. There must exist a function in Cardano.Wallet that can estimate the fees required to join a stake-pool

Development

QA

@jonathanknowles
Copy link
Member

@Anviking, are you able to expand a little more on the following point:

If opportunity arises, we may refactor the functions.

I suspect this is something that could go in the Development section (instead of being an acceptance criterion per se). What do you think?

@jonathanknowles
Copy link
Member

jonathanknowles commented Nov 29, 2019

Regarding the precision of our fee estimation:

When we added support for migration from Byron wallets (to Jörmungandr wallets), we were able to offer a precise fee calculation endpoint (getByronWalletMigrationInfo). Furthermore, we actually test that the predicted value is precisely the same as the eventual value in our integration tests. (See BYRON_MIGRATE_03.)

The reason we can do this is because the wallet UTxO is a value of type Map, and converting any given Map k v to a list with the toList operation always produces a list of [(k, v)] in a deterministic order. (It turns out to be ascending order, but this would work just as well with some other deterministic order.) This order is only dependent on the contents of the map, and independent of the order in which the map was originally constructed. (The order in which the UTxO is deserialized from the database is irrelevant.)

So given that both the estimateJoinStakePoolFee endpoint and joinStakePool both use the same coin selection algorithm, if the UTxO hasn't changed between calling the estimateJoinStakePoolFee function and joinStakePool function, then both ought to produce an identical coin selection, which is equivalent to an identical fee.

Of course, if we elect to perform randomization of the UTxO order in our coin selection algorithm, then the above point won't hold.

@jonathanknowles
Copy link
Member

Regarding the calculation of the fee:

If I'm not mistaken, the fee for a normal transaction seems to be calculated with a composition of the following functions:

  1. estimateSize :: CoinSelection -> Quantity "byte" Int
  2. computeFee :: FeePolicy -> Quantity "byte" Int -> Fee

However, there is also a computeCertFee function in Wallet.Primitive.Fee that we might need to take into account. It's an additional fee on top of the normal transaction fee above.

@KtorZ
Copy link
Member

KtorZ commented Nov 29, 2019

If the UTxO hasn't changed between calling the estimateJoinStakePoolFee function and joinStakePool function, then both ought to produce an identical coin selection

This statement is actually false 🙃 UTxO are selected randomly to cover for fee. That selection may vary from a request to another. Therefore, even for an unchanged UTxO, several calls might result in different selections and different fees.

The case for the Byron migration is very special because, there are no randomness going on: we try to migrate everything.

iohk-bors bot added a commit that referenced this issue Dec 9, 2019
1116: Estimate fees for joining a stake pool r=KtorZ a=Anviking

# Issue Number

WB-32: #1094 #1096 (Found it easier to tackle both at once)

# Overview

Also see commit history, but main points:

- [x] Added API endpoint *GET /stake-pools/{stake-pool}/wallets/{wallet}/fee*
- [x] Added `feeBalance :: CoinSelection -> Word64`. I re-use `selectCoinsForDelegation` together with `feeBalance` to implement the `joinStakePoolFee` handler.
- [x] Integration tests:
    - STAKE_POOLS_ESTIMATE_FEE_01 - fee matches eventual cost
    - STAKE_POOLS_ESTIMATE_FEE_02 - empty wallet cannot estimate fee
    - STAKE_POOLS_ESTIMATE_FEE_03 - can't use byron wallets
    - STAKE_POOLS_ESTIMATE_FEE_04 - invalid pool and wallet ids

<!-- Additional comments or screenshots to attach if any -->

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Assign the PR to a corresponding milestone
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: Johannes Lund <[email protected]>
Co-authored-by: KtorZ <[email protected]>
iohk-bors bot added a commit that referenced this issue Dec 9, 2019
1116: Estimate fees for joining a stake pool r=KtorZ a=Anviking

# Issue Number

WB-32: #1094 #1096 (Found it easier to tackle both at once)

# Overview

Also see commit history, but main points:

- [x] Added API endpoint *GET /stake-pools/{stake-pool}/wallets/{wallet}/fee*
- [x] Added `feeBalance :: CoinSelection -> Word64`. I re-use `selectCoinsForDelegation` together with `feeBalance` to implement the `joinStakePoolFee` handler.
- [x] Integration tests:
    - STAKE_POOLS_ESTIMATE_FEE_01 - fee matches eventual cost
    - STAKE_POOLS_ESTIMATE_FEE_02 - empty wallet cannot estimate fee
    - STAKE_POOLS_ESTIMATE_FEE_03 - can't use byron wallets
    - STAKE_POOLS_ESTIMATE_FEE_04 - invalid pool and wallet ids

<!-- Additional comments or screenshots to attach if any -->

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Assign the PR to a corresponding milestone
 ✓ Acknowledge any changes required to the Wiki
-->


1149: bump versions to v2019.12.9 r=KtorZ a=KtorZ

# Issue Number

<!-- Put here a reference to the issue this PR relates to and which requirements it tackles -->

N/A

# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] I have bumped versions to v2019.12.9

# Comments

<!-- Additional comments or screenshots to attach if any -->

Known issues for the upcoming release:

- #1115
- #1071
- #961

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Assign the PR to a corresponding milestone
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: Johannes Lund <[email protected]>
Co-authored-by: KtorZ <[email protected]>
iohk-bors bot added a commit that referenced this issue Dec 9, 2019
1116: Estimate fees for joining a stake pool r=KtorZ a=Anviking

# Issue Number

WB-32: #1094 #1096 (Found it easier to tackle both at once)

# Overview

Also see commit history, but main points:

- [x] Added API endpoint *GET /stake-pools/{stake-pool}/wallets/{wallet}/fee*
- [x] Added `feeBalance :: CoinSelection -> Word64`. I re-use `selectCoinsForDelegation` together with `feeBalance` to implement the `joinStakePoolFee` handler.
- [x] Integration tests:
    - STAKE_POOLS_ESTIMATE_FEE_01 - fee matches eventual cost
    - STAKE_POOLS_ESTIMATE_FEE_02 - empty wallet cannot estimate fee
    - STAKE_POOLS_ESTIMATE_FEE_03 - can't use byron wallets
    - STAKE_POOLS_ESTIMATE_FEE_04 - invalid pool and wallet ids

<!-- Additional comments or screenshots to attach if any -->

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Assign the PR to a corresponding milestone
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: Johannes Lund <[email protected]>
Co-authored-by: KtorZ <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants