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

Stake address and stake pool delegation certificates #890

Merged
merged 2 commits into from May 9, 2020

Conversation

Jimbo4350
Copy link
Contributor

@Jimbo4350 Jimbo4350 commented May 6, 2020

Issue

  • This PR results in breaking changes to upstream dependencies.

Checklist

  • I have committed clear and descriptive commits. Be considerate as somebody else will have to read these.

  • I have added the appropriate labels to this PR.

@Jimbo4350 Jimbo4350 changed the title Staking key and stake pool delegation certificates Stake key and stake pool delegation certificates May 6, 2020
-- ^ Pool owners.
-> Seq.StrictSeq Shelley.StakePoolRelay
-- ^ Pool relays.
-> Shelley.StrictMaybe Shelley.PoolMetaData
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
-> Shelley.StrictMaybe Shelley.PoolMetaData
-> Shelley.StrictMaybe Shelley.PoolMetaData
-- ^ Pool metadata.

'cause why not 🤷‍♂️

Copy link
Contributor

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

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

I've not done a proper review yet, but the suggestion below should fix the test failure, as well as being the "right thing" TM

cardano-cli/src/Cardano/CLI/Shelley/Parsers.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/Shelley/Run/Transaction.hs Outdated Show resolved Hide resolved
Comment on lines 163 to +164
deriving newtype Show
deriving Eq
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be able to do:

Suggested change
deriving newtype Show
deriving Eq
deriving newtype (Eq, Show)

That is, unless there's some reason for not doing that, that I'm not aware of.

@Jimbo4350 Jimbo4350 added the shelley testnet issues/PRs that need to be done for the Shelley testnet label May 6, 2020
@Jimbo4350 Jimbo4350 force-pushed the jordan/delegation-and-stakepool-certs branch from e5016bb to d65766d Compare May 6, 2020 17:33
dcoutts
dcoutts previously requested changes May 6, 2020
Copy link
Contributor

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

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

Looking good!

A few things below.

cardano-api/src/Cardano/Api.hs Outdated Show resolved Hide resolved
cardano-api/src/Cardano/Api/Types.hs Outdated Show resolved Hide resolved
cardano-api/src/Cardano/Api.hs Outdated Show resolved Hide resolved
cardano-api/src/Cardano/Api.hs Outdated Show resolved Hide resolved
cardano-api/src/Cardano/Api.hs Outdated Show resolved Hide resolved
Comment on lines +139 to +150
= ShelleyDelegationCertificate !ShelleyCertificate
| ShelleyStakePoolCertificate !ShelleyCertificate
deriving (Eq, Show)
Copy link
Contributor

Choose a reason for hiding this comment

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

If these are the same type, do we need two constructors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Jimbo4350 Jimbo4350 force-pushed the jordan/delegation-and-stakepool-certs branch from d65766d to 73b5ea3 Compare May 7, 2020 09:00
@Jimbo4350 Jimbo4350 changed the title Stake key and stake pool delegation certificates Stake address and stake pool delegation certificates May 7, 2020
@Jimbo4350 Jimbo4350 force-pushed the jordan/delegation-and-stakepool-certs branch 4 times, most recently from 1799505 to b2ee7f7 Compare May 9, 2020 10:05
@Jimbo4350 Jimbo4350 force-pushed the jordan/delegation-and-stakepool-certs branch from b2ee7f7 to 196b756 Compare May 9, 2020 10:07
<*> genRewardAccountShelley
<*> genStakePoolOwnersShelley
<*> Gen.list (Range.linear 1 5) genStakePoolRelayShelley
<*> (Just <$> genPoolMetaDataShelley)
Copy link
Contributor

Choose a reason for hiding this comment

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

If that genuinely is optional, you should use Gen.maybe which is Nothing about 30% of the time.

@Jimbo4350 Jimbo4350 dismissed dcoutts’s stale review May 9, 2020 11:25

requested changes made

…ates

Integrate delegation and stake pool certificates in `buildShelleyTransaction`
@Jimbo4350 Jimbo4350 force-pushed the jordan/delegation-and-stakepool-certs branch from 196b756 to 9df4f1c Compare May 9, 2020 11:27
@Jimbo4350
Copy link
Contributor Author

bors merge

iohk-bors bot added a commit that referenced this pull request May 9, 2020
890: Stake address and stake pool delegation certificates r=Jimbo4350 a=Jimbo4350

Issue
-----------

- This PR **results** in breaking changes to upstream dependencies.

Checklist
---------

- [x] I have committed clear and descriptive commits. Be considerate as somebody else will have to read these.

- [x] I have added the appropriate labels to this PR.


Co-authored-by: Jordan Millar <jordan.millar@iohk.io>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 9, 2020

Timed out

@Jimbo4350
Copy link
Contributor Author

bors merge

@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 9, 2020

@iohk-bors iohk-bors bot merged commit 494f1fa into master May 9, 2020
@iohk-bors iohk-bors bot deleted the jordan/delegation-and-stakepool-certs branch May 9, 2020 14:10
iohk-bors bot added a commit that referenced this pull request May 9, 2020
895: Genesis stake address and MIR certificates r=Jimbo4350 a=Jimbo4350

Issue
-----------
- Depends on #890 
- This PR **does not result** in breaking changes to upstream dependencies.

Checklist
---------

- [x] I have committed clear and descriptive commits. Be considerate as somebody else will have to read these.

- [x] I have added the appropriate labels to this PR.


Co-authored-by: Jordan Millar <jordan.millar@iohk.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
shelley testnet issues/PRs that need to be done for the Shelley testnet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants