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

Scan stake pool registrations during wallet restoration. #1050

Merged

Conversation

jonathanknowles
Copy link
Member

@jonathanknowles jonathanknowles commented Nov 20, 2019

Issue Number

#899

Overview

I have:

  • Provided an implementation of IsOurs (SeqState n k) ChimericAccount.
  • Adjusted the return type of applyBlock so that it references any stake delegation that has occurred on behalf of the specified wallet.
  • Adjusted the definition of restoreBlocks so that it correctly processes the registrations returned by applyBlocks.
  • Adjusted the definition of applyBlock to identify delegations relevant to the specified wallet, and return the final registration as part of FilteredBlock.
  • Added tests to ensure that applyBlock does successfully discover delegations made by the wallet under test, and only that wallet.
  • Added tests to ensure that a wallet's delegation status is correctly updated in the context of discovered delegations.

@jonathanknowles jonathanknowles self-assigned this Nov 20, 2019
@jonathanknowles jonathanknowles force-pushed the jonathanknowles/scan-pool-registrations-during-restoration branch from 75111f5 to 0a6850d Compare November 21, 2019 04:26
@jonathanknowles jonathanknowles force-pushed the jonathanknowles/scan-pool-registrations-during-restoration branch from 0a6850d to 51bb22f Compare November 21, 2019 05:14
@KtorZ
Copy link
Member

KtorZ commented Nov 21, 2019

@jonathanknowles why is this one still a draft + WIP ?
Also, the PR description is somewhat ... missing 😬

Is it actually ready for review, what are the next steps?

@jonathanknowles
Copy link
Member Author

@jonathanknowles why is this one still a draft + WIP ?

Because I had put this on hold (temporarily) to work on other things. (Namely: PR #1057).

Also, the PR description is somewhat ... missing

Understood. I'll update this shortly.

Is it actually ready for review.

Not yet.

what are the next steps?

Three things:

  1. To adjust the definition of applyBlock to identify delegations relevant to the specified wallet, and return the final registration as part of ApplyBlockResult.
  2. To adjust the definition of restoreBlocks so that it correctly processes the registrations returned by applyBlocks.
  3. Tests for the above.

@jonathanknowles jonathanknowles force-pushed the jonathanknowles/scan-pool-registrations-during-restoration branch from 51bb22f to 84bf451 Compare November 25, 2019 06:29
@jonathanknowles jonathanknowles changed the base branch from jonathanknowles/block-delegations to master November 25, 2019 08:53
@jonathanknowles jonathanknowles force-pushed the jonathanknowles/scan-pool-registrations-during-restoration branch 13 times, most recently from 2220460 to 3f7de4c Compare November 28, 2019 06:41
@jonathanknowles jonathanknowles force-pushed the jonathanknowles/scan-pool-registrations-during-restoration branch from 3f7de4c to 66a085d Compare November 28, 2019 07:44
@jonathanknowles jonathanknowles force-pushed the jonathanknowles/scan-pool-registrations-during-restoration branch from 94e7d01 to 75e139a Compare November 28, 2019 08:08
Copy link
Member

@KtorZ KtorZ left a comment

Choose a reason for hiding this comment

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

LGTM

lib/core/src/Cardano/Wallet.hs Show resolved Hide resolved
lib/core/src/Cardano/Wallet/Primitive/Model.hs Outdated Show resolved Hide resolved
isOurs (ChimericAccount account) state =
(account == ourAccount, state)
where
ourAccount = xpubPublicKey $ getRawKey $ rewardAccountKey state
Copy link
Member

Choose a reason for hiding this comment

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

👌

jonathanknowles and others added 12 commits November 28, 2019 13:14
Fix comment for function `putDelegationCertificate`.
Fix comment for function `applyBlock`.
Fix comment for function `prefilterBlock`.
Use this type to replace the list of transactions returned by
`applyBlock` and `applyBlocks`.

Future changes will add more fields to this type.
This represents the set of stake delegations made on behalf of the
wallet, listed in order of discovery.
For the moment, we return an empty list for the list of delegations.
When restoring blocks.

Additionally, write a log entry for each certificate discovered.
@KtorZ KtorZ force-pushed the jonathanknowles/scan-pool-registrations-during-restoration branch from a676584 to 2c31cbd Compare November 28, 2019 12:54
@KtorZ KtorZ marked this pull request as ready for review November 28, 2019 12:55
@KtorZ KtorZ changed the title WIP: Scan stake pool registrations during wallet restoration. Scan stake pool registrations during wallet restoration. Nov 28, 2019
@KtorZ
Copy link
Member

KtorZ commented Nov 28, 2019

I've noted a quite important change to do when storing the delegation after discovery, but I'll make it another PR to keep this one concise. It doesn't affect the core functionality, but is important in terms of reliability and data consistency.

@KtorZ
Copy link
Member

KtorZ commented Nov 28, 2019

bors r+

iohk-bors bot added a commit that referenced this pull request Nov 28, 2019
1050: Scan stake pool registrations during wallet restoration. r=KtorZ a=jonathanknowles

# Issue Number

#899 

# Overview

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

I have:

- [x] Provided an implementation of `IsOurs (SeqState n k) ChimericAccount`.
- [x] Adjusted the return type of `applyBlock` so that it references any stake delegation that has occurred on behalf of the specified wallet.
- [x] Adjusted the definition of `restoreBlocks` so that it correctly processes the registrations returned by `applyBlocks`.
- [x] Adjusted the definition of `applyBlock` to identify delegations relevant to the specified wallet, and return the final registration as part of `FilteredBlock`.
- [x] Added tests to ensure that `applyBlock` does successfully discover delegations made by the wallet under test, and only that wallet.
- [x]  Added tests to ensure that a wallet's delegation status is correctly updated in the context of discovered delegations.

<!-- 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
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: Jonathan Knowles <[email protected]>
Co-authored-by: KtorZ <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Nov 28, 2019

Build failed

@paweljakubas paweljakubas mentioned this pull request Nov 28, 2019
6 tasks
@KtorZ
Copy link
Member

KtorZ commented Nov 28, 2019

bors r+

iohk-bors bot added a commit that referenced this pull request Nov 28, 2019
1050: Scan stake pool registrations during wallet restoration. r=KtorZ a=jonathanknowles

# Issue Number

#899 

# Overview

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

I have:

- [x] Provided an implementation of `IsOurs (SeqState n k) ChimericAccount`.
- [x] Adjusted the return type of `applyBlock` so that it references any stake delegation that has occurred on behalf of the specified wallet.
- [x] Adjusted the definition of `restoreBlocks` so that it correctly processes the registrations returned by `applyBlocks`.
- [x] Adjusted the definition of `applyBlock` to identify delegations relevant to the specified wallet, and return the final registration as part of `FilteredBlock`.
- [x] Added tests to ensure that `applyBlock` does successfully discover delegations made by the wallet under test, and only that wallet.
- [x]  Added tests to ensure that a wallet's delegation status is correctly updated in the context of discovered delegations.

<!-- 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
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: Jonathan Knowles <[email protected]>
Co-authored-by: KtorZ <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Nov 28, 2019

Build succeeded

@iohk-bors iohk-bors bot merged commit 9c23350 into master Nov 28, 2019
@KtorZ KtorZ deleted the jonathanknowles/scan-pool-registrations-during-restoration branch November 28, 2019 14:46
iohk-bors bot added a commit that referenced this pull request Nov 29, 2019
1080: Implement quit stake pool r=paweljakubas a=paweljakubas

# Issue Number

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

# Overview

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

- [x] I have handled NotDelegating case
- [x] I have handled NoSuchPool case
- [x] I have renamed mkDelegationCertTx to mkDelegationJoinTx and added new mkDelegationQuitTx
- [x] I have added corresponding golden tests (if possible)
- [x] I have reused create/sign functions in quitStakePool func
- [x] I have adjusted integration tests (after #1050 is merged)


# Comments

<!-- Additional comments or screenshots to attach if any -->
I do not think it is easy to add corresponding golden tests here
 
<!-- 
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: Pawel Jakubas <[email protected]>
@KtorZ KtorZ added this to the Support Delegated Addresses milestone Dec 9, 2019
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

Successfully merging this pull request may close these issues.

2 participants