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

Extend 'Block' to also contain delegation certificates registrations #898

Closed
3 tasks done
KtorZ opened this issue Oct 23, 2019 · 7 comments
Closed
3 tasks done

Extend 'Block' to also contain delegation certificates registrations #898

KtorZ opened this issue Oct 23, 2019 · 7 comments
Assignees

Comments

@KtorZ
Copy link
Member

KtorZ commented Oct 23, 2019

Context

Delegation certificate registration (a pair of a pool id and an account public key) can be present in blocks. We'll have to be able to process them and adjust the wallet delegation settings accordingly.

Decision

  • Extend primitive 'Block' type to also contain delegation certificates registrations. e.g.

    data Block tx = Block
        { header
            :: !BlockHeader
        , transactions
            :: ![tx]
        , delegations
            :: ![(XPub, PoolId)]
        } 
  • Extend Jörmungandr Binary decoder to decode certificate messages & adjust conversion functions from Jörmungandr 'Block' to our new primitive 'Block'.

Acceptance Criteria

  1. Cardano.Wallet.Primitive.Types#Block must contain delegation registrations requests
  2. Cardano.Wallet.Jormungandr.Binary#Block must contain delegation registrations requests
  3. cardano-wallet-jormungandr must support correctly map Type 3 messages and Type 1 messages to primitive Block

Development Plan

  • Extend the Jörmungandr Block type to contain stake delegations.
  • Adjust the convertBlock function to populate the delegations field when converting from a Jörmungandr block to Wallet-layer block.
  • Write golden tests for convertBlock with golden blocks with delegations.

PR

Number Base
#1043 master

QA

Covered by QA tasks in issue #899.

@jonathanknowles
Copy link
Member

jonathanknowles commented Nov 18, 2019

Hi @KtorZ, are you able to say what "Type 3" and "Type 1" refer to in the acceptance criterion below:

  1. cardano-wallet-jormungandr must support correctly map Type 3 messages and Type 1 messages to primitive Block

I had a look through and couldn't see anything obvious. Thanks in advance!

@KtorZ
Copy link
Member Author

KtorZ commented Nov 18, 2019

@jonathanknowles this refers to the fragment type (a.k.a fragment spec in Jormungandr).
Yet, since I wrote this ticket, type 3 became type 4 :)

What's important to capture here is that delegation certificates should now be part of our primitive blocks, as the title suggests.

iohk-bors bot added a commit that referenced this issue Nov 25, 2019
1043: Extend `Block` type to contain delegation certificate registrations. r=jonathanknowles a=jonathanknowles

# Issue Number

#898 

# Overview

I have:

- [x] Extended the Jörmungandr `Block` type to contain stake delegations.
- [x] Adjusted the `convertBlock` function to populate the `delegations` field when converting from a Jörmungandr block to Wallet-layer block.

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


Co-authored-by: Jonathan Knowles <[email protected]>
@KtorZ
Copy link
Member Author

KtorZ commented Nov 25, 2019

*Putting this back to In progress because:

  • We're missing the PRs links into to PR section
  • The QA section is empty.

@jonathanknowles
Copy link
Member

@KtorZ in your opinion, are additional tests required over and above the tests already provided by #899? (PR #1050).

We could of course provide additional unit or property tests for the convertBlock function.

@KtorZ
Copy link
Member Author

KtorZ commented Dec 6, 2019

I don't think so no. With all the integration tests for this one and the binary unit tests, it seems to me that it's already well tested. 1

@jonathanknowles
Copy link
Member

@KtorZ wrote:

I don't think so no. With all the integration tests for this one and the binary unit tests, it seems to me that it's already well tested. 1

Okay in that case, I'll adjust the QA section to refer to the tests produced as part of fixing issue #899.

Thanks for confirming!

@piotr-iohk
Copy link
Contributor

lgtm.

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

3 participants