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.

Sign up for GitHub

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

Jump to bottom

Add API endpoint for delegation fee estimation #1096

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

Add API endpoint for delegation fee estimation #1096

Anviking opened this issue Nov 29, 2019 · 7 comments
Assignees
@Anviking
Milestone
(WB-32) Estimate ...

Comments

@Anviking
Copy link
Member

Anviking commented Nov 29, 2019 •
edited by KtorZ
Loading

Context

Users of the API would like to know the cost of joining a stake-pool before pulling the trigger.

In this task we implement the API, leveraging the work of #1094.

Decision

Add the following endpoint:

/wallets/\{walletId\}/delegations/fees: 
    get: 
      operationId: getDelegationFee
      tags: ["Stake Pools"]
      summary: Estimate Fee
      parameters: 
        - *parametersWalletId
      responses: *responsesGetDelegationFee

x-responsesGetDelegationFee: &responsesGetDelegationFee
  <<: *responsesErr403
  <<: *responsesErr404
  <<: *responsesErr405
  <<: *responsesErr406
  200:
    description: Ok
    schema: *ApiFee

Acceptance Criteria

  • The endpoint must be implemented
  • JSON roundtrip tests for any added Api-types should exist
  • The endpoint should be tested with integration tests

Development

QA
@Anviking Anviking self-assigned this Nov 29, 2019
@jonathanknowles
Copy link
Member

jonathanknowles commented Nov 29, 2019 •
edited
Loading

We might want to add integration tests to check that:

  • The eventual fee paid for joining a stake pool is similar or identical to the estimated delegation fee (similar to BYRON_MIGRATE_03).
  • The estimated delegation fee for a non-empty wallet is > 0 (similar to BYRON_CALCULATE_01).
  • Attempting to calculate a delegation fee for an empty wallet produces an appropriate error (similar to BYRON_CALCULATE_02).
  • Attempting to calculate a delegation fee for a non-Shelley wallet produces an appropriate error (similar to BYRON_CALCULATE_03).
  • Attempting to calculate a delegation fee for a non-existing wallet produces an appropriate error (similar to BYRON_CALCULATE_04).

@jonathanknowles
Copy link
Member

jonathanknowles commented Nov 29, 2019 •
edited
Loading

@KtorZ I've added a question regarding the original requirement here: https://jira.iohk.io/browse/WB-32?focusedCommentId=11089&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-11089

(The question is aimed at clarifying whether or not this endpoint should really be an HTTP GET operation, or whether the endpoint really does require a user-supplied payload, in which case we will indeed need a POST operation.)

@Anviking Anviking mentioned this issue Dec 4, 2019
Merged
3 tasks
iohk-bors bot added a commit that referenced this issue Dec 9, 2019
@iohk-bors @Anviking @KtorZ
Merge #1116
04e2bd5 
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]>
@KtorZ KtorZ changed the title Add API endpoint for join-stake-pool fee estimation Add API endpoint for delegation fee estimation Dec 9, 2019
iohk-bors bot added a commit that referenced this issue Dec 9, 2019
@iohk-bors @Anviking @KtorZ
Merge #1116 #1149
4d11083 
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
@iohk-bors @Anviking @KtorZ
Merge #1116
dbd5a2c 
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]>
@piotr-iohk
Copy link
Contributor

Few more tests would be handy on negative cases side of things. Also there is no assertion check for the 200 http status code for most basic fee request.

@piotr-iohk
Copy link
Contributor

Fee estimation also does not show correct amount -> #1150 (the amount does not correspond with genesis.yaml parameters -> some more info also here https://input-output-rnd.slack.com/archives/GBT05825V/p1575876039070100)

@Anviking Anviking mentioned this issue Dec 10, 2019
Merged
2 tasks
@Anviking
Copy link
Member Author

Few more tests would be handy on negative cases side of things

@piotr-iohk what kind of negative tests would you like to see? There are already three

iohk-bors bot added a commit that referenced this issue Dec 11, 2019
@iohk-bors @Anviking
Merge #1156
c51fd5c 
1156: Expect 200 for fee-estimation r=KtorZ a=Anviking

# Issue Number

#1096 


# Overview

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

- [x] I have added a `expectResponseCode HTTP.status200` for *STAKE_POOLS_ESTIMATE_FEE_01*
- [x] I tweaked the description of *STAKE_POOLS_ESTIMATE_FEE_04*


# Comments

<!-- 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]>
@piotr-iohk piotr-iohk mentioned this issue Dec 12, 2019
Merged
1 task
@piotr-iohk
Copy link
Contributor

@piotr-iohk what kind of negative tests would you like to see? There are already three

They are good indeed!
Just added 2 more to test against 405 and 406 -> #1170.

iohk-bors bot added a commit that referenced this issue Dec 12, 2019
@iohk-bors
Merge #1170
c89ec5d 
1170: STAKE_POOLS_ESTIMATE_FEE negative tests for 40x responses r=Anviking a=piotr-iohk

# Issue Number

#1096

# Overview

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

- [ ] I have added few more tests  for 40x responses


# Comments

<!-- 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: Piotr Stachyra <[email protected]>
iohk-bors bot added a commit that referenced this issue Dec 12, 2019
@iohk-bors
Merge #1170
6f09452 
1170: STAKE_POOLS_ESTIMATE_FEE negative tests for 40x responses r=Anviking a=piotr-iohk

# Issue Number

#1096

# Overview

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

- [ ] I have added few more tests  for 40x responses


# Comments

<!-- 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: Piotr Stachyra <[email protected]>
iohk-bors bot added a commit that referenced this issue Dec 12, 2019
@iohk-bors
Merge #1170
ce3d4ae 
1170: STAKE_POOLS_ESTIMATE_FEE negative tests for 40x responses r=Anviking a=piotr-iohk

# Issue Number

#1096

# Overview

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

- [ ] I have added few more tests  for 40x responses


# Comments

<!-- 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: Piotr Stachyra <[email protected]>
@piotr-iohk
Copy link
Contributor

ok.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Anviking Anviking

Labels
None yet
Projects
None yet
Milestone
(WB-32) Estimate cost of delegation
Development

No branches or pull requests
3 participants
@jonathanknowles @Anviking @piotr-iohk