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

Prepare room for Shelley addresses #1669

Merged
merged 8 commits into from
May 20, 2020
Merged

Prepare room for Shelley addresses #1669

merged 8 commits into from
May 20, 2020

Conversation

KtorZ
Copy link
Member

@KtorZ KtorZ commented May 15, 2020 •
edited
Loading

Issue Number

#1672

Overview

  • f98b904
    📍 cleanup redundant constraints from the wallet engine

  • 9524ccc
    📍 remove some hard-coded 'ShelleyKey' in favor of more flexible constraints

  • c8bada8
    📍 rename AddressDerivation.Shelley & ShelleyKey to Jormungandr
    Better reflect what this module really is and what it really targets. Makes room for the iminent shelley address support.

  • 5bd26c6
    📍 Regenerate nix

  • 2c27166
    📍 add partial implementation for Shelley keys, for type-checking

  • a767ac2
    📍 replace occurences of 'ShelleyKey' with 'JormungandrKey' in relevant integration tests

  • 9d5c3fd
    📍 implement bits about fingerprints for ShelleyKey

Comments

Based on #1664

@KtorZ KtorZ added the ADDING FEATURE Mark a PR as adding a new feature, for auto-generated CHANGELOG label May 15, 2020
@KtorZ KtorZ self-assigned this May 15, 2020
@Anviking Anviking force-pushed the KtorZ/rename-jormungandr-shelley branch from 31d42c3 to 3ccb18f Compare May 18, 2020 14:47
@paweljakubas paweljakubas force-pushed the paweljakubas/1658/add-sendAll-endpoint branch from eeeabfc to 71433ba Compare May 18, 2020 17:42
@Anviking Anviking force-pushed the KtorZ/rename-jormungandr-shelley branch from f1dc781 to 897613f Compare May 19, 2020 09:25
@KtorZ KtorZ force-pushed the KtorZ/rename-jormungandr-shelley branch from 897613f to a767ac2 Compare May 19, 2020 11:54
@KtorZ KtorZ changed the base branch from paweljakubas/1658/add-sendAll-endpoint to master May 19, 2020 12:00
paweljakubas
paweljakubas reviewed May 19, 2020
View reviewed changes
lib/cli/src/Cardano/CLI.hs
@@ -344,7 +344,7 @@ import qualified Cardano.BM.Data.BackendKind as CM
import qualified Cardano.Crypto.Wallet as CC
import qualified Cardano.Wallet.Api.Types as API
import qualified Cardano.Wallet.Primitive.AddressDerivation.Icarus as Icarus
import qualified Cardano.Wallet.Primitive.AddressDerivation.Shelley as Shelley
import qualified Cardano.Wallet.Primitive.AddressDerivation.Jormungandr as Jormungandr
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

paweljakubas
paweljakubas reviewed May 19, 2020
View reviewed changes
lib/core/src/Cardano/Wallet/Api/Server.hs
import qualified Cardano.Wallet.Primitive.AddressDerivation.Byron as Rnd
import qualified Cardano.Wallet.Primitive.AddressDerivation.Icarus as Ica
import qualified Cardano.Wallet.Primitive.AddressDerivation.Shelley as Seq
import qualified Cardano.Wallet.Primitive.AddressDerivation.Byron as Byron
Copy link
Contributor

Choose a reason for hiding this comment

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

good to have proper/precise naming finally

paweljakubas
paweljakubas reviewed May 19, 2020
View reviewed changes
lib/core/src/Cardano/Wallet/Primitive/AddressDerivation/Shelley.hs
putByteString . blake2b224 $ paymentK
where
enterprise = 96
networkId = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

I see you adopted already what is going to be valid in cardano-ledger-spec in a while. 0 network id for testnet 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

According to what was last communicated indeed, although it's probably not yet merged on their side.

paweljakubas
paweljakubas reviewed May 19, 2020
View reviewed changes
lib/core/src/Cardano/Wallet/Primitive/AddressDerivation/Shelley.hs
putByteString . blake2b224 $ stakingK
where
base = 0
networkId = 1
Copy link
Contributor

Choose a reason for hiding this comment

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

network id =1 is only one possible mainnet, it can be form 1 to 15 (if I understand it well). we are ok now with fixed one value?

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe so. We have some time to see the other mainnets coming, I hope 😶

paweljakubas
paweljakubas reviewed May 19, 2020
View reviewed changes
lib/jormungandr/src/Cardano/Wallet/Jormungandr/Api/Server.hs
:<|> putWallet shelley mkShelleyWallet
:<|> putWalletPassphrase shelley
:<|> getUTxOsStatistics shelley
wallets = deleteWallet jormungandr
Copy link
Contributor

Choose a reason for hiding this comment

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

now it read much better !

paweljakubas
paweljakubas approved these changes May 19, 2020
View reviewed changes
Copy link
Contributor

@paweljakubas paweljakubas left a comment

Choose a reason for hiding this comment

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

Looks like Shelley got its proper place in the code, other naming bits that were ambiguous (like Rnd but it should be Byron, Shelley that it should be Jormungandr) were addressed. Hope to see one day, the more extensive usage of cardano-addresses style implementations ;-)

@KtorZ KtorZ force-pushed the KtorZ/rename-jormungandr-shelley branch from 325abcf to fbbae4d Compare May 19, 2020 13:10
Anviking
Anviking approved these changes May 20, 2020
View reviewed changes
Copy link
Member

@Anviking Anviking left a comment

Choose a reason for hiding this comment

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

🎉

@KtorZ
Copy link
Member Author

KtorZ commented May 20, 2020

Buildkite hydra-eval failed, but the new ci/hydra-eval succeeded. Which one should I trust 😱

@KtorZ KtorZ marked this pull request as ready for review May 20, 2020 09:41
@KtorZ
Copy link
Member Author

KtorZ commented May 20, 2020

bors r+

1 similar comment
@KtorZ
Copy link
Member Author

KtorZ commented May 20, 2020

bors r+

iohk-bors bot added a commit that referenced this pull request May 20, 2020
@iohk-bors @KtorZ
Merge #1669
f397080 
1669: Prepare room for Shelley addresses r=KtorZ a=KtorZ

# Issue Number

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

#1672

# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->
- f98b904
  📍 **cleanup redundant constraints from the wallet engine**
  
- 9524ccc
  📍 **remove some hard-coded 'ShelleyKey' in favor of more flexible constraints**
  
- c8bada8
  📍 **rename AddressDerivation.Shelley & ShelleyKey to Jormungandr**
  Better reflect what this module really is and what it really targets. Makes room for the iminent shelley address support.

- 5bd26c6
  📍 **Regenerate nix**
  
- 2c27166
  📍 **add partial implementation for Shelley keys, for type-checking**
  
- a767ac2
  📍 **replace occurences of 'ShelleyKey' with 'JormungandrKey' in relevant integration tests**
  
- 9d5c3fd
  📍 **implement bits about fingerprints for ShelleyKey**

# Comments

<!-- Additional comments or screenshots to attach if any -->

Based on #1664 

<!-- 
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: KtorZ <matthias.benkort@gmail.com>
Co-authored-by: IOHK <devops+stack-project@iohk.io>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 20, 2020

Build failed

@rvl
Copy link
Contributor

rvl commented May 20, 2020

Oops, I broke the Hydra build. That's why bors is not working. The fix is here #1661. I will merge this now since it passed everything.

@rvl rvl merged commit 7dc7ec7 into master May 20, 2020
@rvl rvl deleted the KtorZ/rename-jormungandr-shelley branch May 20, 2020 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paweljakubas paweljakubas paweljakubas approved these changes

@Anviking Anviking Anviking approved these changes

Assignees

@KtorZ KtorZ

Labels
ADDING FEATURE Mark a PR as adding a new feature, for auto-generated CHANGELOG
Projects
None yet
Milestone
No milestone
Development

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