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 GET /wallets/{walletId}/stake-keys endpoint #2666

Merged
merged 1 commit into from
May 28, 2021
Merged

Add GET /wallets/{walletId}/stake-keys endpoint #2666

merged 1 commit into from
May 28, 2021

Conversation

Anviking
Copy link
Member

@Anviking Anviking commented May 25, 2021 •
edited
Loading

Issue Number

ADP-721

Overview

  • Add GET /wallets/{wid}/stake-keys endpoint
  • Provide information about stake-key 0
  • Show the distribution of ada across different stake keys

Comments

Skärmavbild 2021-05-25 kl 13 01 30

@Anviking Anviking force-pushed the anviking/ADP-721/list-stake-keys branch from 518d6d8 to 22bfb2a Compare May 25, 2021 08:36
Anviking
Anviking commented May 25, 2021
View reviewed changes
lib/core/src/Cardano/Wallet/Api/Types.hs Outdated
@@ -1426,6 +1431,42 @@ newtype ApiMnemonicT (sizes :: [Nat]) =
ApiMnemonicT { getApiMnemonicT :: SomeMnemonic }
deriving (Generic, Show, Eq)


data ApiOurStakeKey n = ApiOurStakeKey
{ _index :: ApiT DerivationIndex
Copy link
Member Author

@Anviking Anviking May 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems DerivationIndex is encoded as a JSON string, which seems unnecessary. Going with Natural then, I guess. Could maybe have some ApiSoftIndex newtype if desired…

@Anviking Anviking self-assigned this May 25, 2021
KtorZ
KtorZ reviewed May 25, 2021
View reviewed changes
lib/core/src/Cardano/Wallet/Api/Types.hs Outdated
data ApiStakeKeys n = ApiStakeKeys
{ _ours :: ![ApiOurStakeKey n]
, _foreign :: ![ApiForeignStakeKey n]
, _null :: !ApiNullStakeKey
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

null being a special keyword in JavaScript and JSON, I'd highly recommend to NOT using null as a field name! Perhaps none could work a bit better or inactive (though stake can be associated with a stake key and still be inactive.. so perhaps it is a poor choice).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, fair! Probably none then

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another thing: I think currently the balance of say script and pointer addresses get included into this none, which could be really confusing if it happens...

none should probably only correspond to enterprise addresses.

Then we could have another bucket for "other/unknown", or leave it out for now.

Copy link
Member Author

@Anviking Anviking May 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we could have another bucket for "other/unknown", or leave it out for now.

For now I just mentioned that none could include more things than enterprise addresses in the swagger API. There is no clear use-case besides testing/debugging, so I think this is fine as long as it is mentioned.

rvl
rvl approved these changes May 25, 2021
View reviewed changes
Copy link
Contributor

@rvl rvl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty sensible 👍

lib/core/src/Cardano/Wallet/Api/Server.hs Outdated
-> ctx
-> ApiT WalletId
-> Handler (ApiStakeKeys n)
listStakeKeys stakeRef ctx (ApiT wid) = do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is quite a big function. Is it at all possible to break it up a little? Then it might be possible to unit test some of it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 The thought occurred to me, but wasn't sure if worth it; thanks for suggesting, will give it a shot

Copy link
Member Author

@Anviking Anviking May 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I split off a helper that could run in Identity. But to actually unit-test it, I can't get a way with only defining Api types like I do here. Will do in a separate PR.

Edit: although those attempts didn't go great, so maybe not.

@Anviking Anviking force-pushed the anviking/ADP-721/list-stake-keys branch from dea7b6e to 58075a5 Compare May 28, 2021 10:17
@Anviking
Copy link
Member Author

bors r+

iohk-bors bot added a commit that referenced this pull request May 28, 2021
@iohk-bors @Anviking
Merge #2666
e9a3db5 
2666: Add GET /wallets/{walletId}/stake-keys endpoint r=Anviking a=Anviking

# Issue Number

<!-- Put here a reference to the issue that this PR relates to and which requirements it tackles. Jira issues of the form ADP- will be auto-linked. -->

ADP-721


# Overview

- [x] Add `GET /wallets/{wid}/stake-keys` endpoint
- [x] Provide information about stake-key 0
- [x] Show the distribution of ada across different stake keys

# Comments

<img width="1238" alt="Skärmavbild 2021-05-25 kl  13 01 30" src="https://user-images.githubusercontent.com/304423/119487609-e6643d00-bd59-11eb-835d-49e42057fac2.png">


<!-- 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)
 ✓ Jira will detect and link to this PR once created, but you can also link this PR in the description of the corresponding ticket
 ✓ Acknowledge any changes required to the Wiki
 ✓ Finally, in the PR description delete any empty sections and all text commented in <!--, so that this text does not appear in merge commit messages.
-->


Co-authored-by: Johannes Lund <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 28, 2021

Build failed:

@Anviking
Add GET /wallets/{walletId}/stake-keys endpoint
de38a17 
It provides information on:
- The wallet's stake keys (now 1, later many)
- Stake keys that appear in the UTxO, but are not ours
- Breakdown of how much ada is associated with which key, and how much
ada is not associated with any key at all.

The goal is to provide enough information for the multi-delegation
Proof-of-Concept.

Example JSON:

```
{
    "foreign": [],
    "none": {
        "stake": {
            "quantity": 0,
            "unit": "lovelace"
        }
    },
    "ours": [
        {
            "delegation": {
                "active": {
                    "status": "not_delegating"
                },
                "next": []
            },
            "index": 0,
            "key": "stake_test1ur7tw2l4t0d4dcdg46rnlw58axcqa0rlju0tcak0f50usnq9xtnxm",
            "reward_balance": {
                "quantity": 0,
                "unit": "lovelace"
            },
            "stake": {
                "quantity": 1000000000,
                "unit": "lovelace"
            }
        }
    ]
}
```

fixup: use Natural instead of DerivationIndex
fixup: make fields strict
fixup: inline delegationBalance
fixup: docs and api docs
Rename null -> none, tweak api docs
fixup to fixup: rename to none
factor out a more testable listStakeKeys helper
Minor polish
fixup golden
@Anviking Anviking force-pushed the anviking/ADP-721/list-stake-keys branch from c1083e3 to de38a17 Compare May 28, 2021 13:21
@Anviking Anviking marked this pull request as ready for review May 28, 2021 13:22
@Anviking
Copy link
Member Author

bors r+

iohk-bors bot added a commit that referenced this pull request May 28, 2021
@iohk-bors @Anviking
Merge #2666
1cf0926 
2666: Add GET /wallets/{walletId}/stake-keys endpoint r=Anviking a=Anviking

# Issue Number

<!-- Put here a reference to the issue that this PR relates to and which requirements it tackles. Jira issues of the form ADP- will be auto-linked. -->

ADP-721


# Overview

- [x] Add `GET /wallets/{wid}/stake-keys` endpoint
- [x] Provide information about stake-key 0
- [x] Show the distribution of ada across different stake keys

# Comments

<img width="1238" alt="Skärmavbild 2021-05-25 kl  13 01 30" src="https://user-images.githubusercontent.com/304423/119487609-e6643d00-bd59-11eb-835d-49e42057fac2.png">


<!-- 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)
 ✓ Jira will detect and link to this PR once created, but you can also link this PR in the description of the corresponding ticket
 ✓ Acknowledge any changes required to the Wiki
 ✓ Finally, in the PR description delete any empty sections and all text commented in <!--, so that this text does not appear in merge commit messages.
-->


Co-authored-by: Johannes Lund <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 28, 2021

Build failed:

@Anviking
Copy link
Member Author

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 28, 2021

Build succeeded:

@iohk-bors iohk-bors bot merged commit d659651 into master May 28, 2021
@iohk-bors iohk-bors bot deleted the anviking/ADP-721/list-stake-keys branch May 28, 2021 17:03
WilliamKingNoel-Bot pushed a commit that referenced this pull request May 28, 2021
docs: Merge #2666
1de91c1 
2666: Add GET /wallets/{walletId}/stake-keys endpoint r=Anviking a=Anviking

# Issue Number

<!-- Put here a reference to the issue that this PR relates to and which requirements it tackles. Jira issues of the form ADP- will be auto-linked. -->

ADP-721

# Overview

- [x] Add `GET /wallets/{wid}/stake-keys` endpoint
- [x] Provide information about stake-key 0
- [x] Show the distribution of ada across different stake keys

# Comments

<img width="1238" alt="Skärmavbild 2021-05-25 kl  13 01 30" src="https://user-images.githubusercontent.com/304423/119487609-e6643d00-bd59-11eb-835d-49e42057fac2.png">

<!-- 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)
 ✓ Jira will detect and link to this PR once created, but you can also link this PR in the description of the corresponding ticket
 ✓ Acknowledge any changes required to the Wiki
 ✓ Finally, in the PR description delete any empty sections and all text commented in <!--, so that this text does not appear in merge commit messages.
-->

Co-authored-by: Johannes Lund <[email protected]> d659651
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KtorZ KtorZ KtorZ left review comments

@rvl rvl rvl approved these changes

Assignees

@Anviking Anviking

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Anviking @rvl @KtorZ