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

ADP-2574 feat/crypto library interface and key management refactor #589

Merged
merged 6 commits into from
Feb 6, 2023
Merged

ADP-2574 feat/crypto library interface and key management refactor #589

merged 6 commits into from
Feb 6, 2023

Conversation

AngelCastilloB
Copy link
Member

@AngelCastilloB AngelCastilloB commented Jan 30, 2023 •
edited
Loading

Context

We must add an abstraction for the crypto operations for the key management package to depend on, so we can do comparison tests between implementations.

Proposed Solution

Add an interface and extension point around the cryptographic operations in the SDK.

Important Changes Introduced

  • Added a new interface Crypto package.
  • Added a new Ed25519Provider interface to abstract cryptographic operations.
  • Added a new Cryptographic provider implemented in terms of the CML library.
  • The key-management package was refactored to use the new abstraction.

@AngelCastilloB AngelCastilloB force-pushed the feat/crypto-library-interface-and-key-management-refactor branch 2 times, most recently from 1f11838 to 839183e Compare January 31, 2023 00:26
mkazlauskas
mkazlauskas requested changes Jan 31, 2023
View reviewed changes
Copy link
Member

@mkazlauskas mkazlauskas left a comment

Choose a reason for hiding this comment

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

Please highlight changes to the interface in e1a6b10 commit message (new required 'cryptoProvider' property in some types)

packages/crypto/src/Ed25519Provider.ts Outdated Show resolved Hide resolved
packages/crypto/src/Ed25519Provider.ts Outdated Show resolved Hide resolved
packages/crypto/src/Ed25519Provider.ts Outdated Show resolved Hide resolved
packages/crypto/src/Ed25519Provider.ts Outdated Show resolved Hide resolved
packages/crypto/src/Ed25519Provider.ts Outdated Show resolved Hide resolved
packages/crypto/src/Providers/CmlEd25519Provider.ts Outdated Show resolved Hide resolved
packages/crypto/src/Providers/CmlEd25519Provider.ts Outdated Show resolved Hide resolved
packages/crypto/src/errors.ts Outdated Show resolved Hide resolved
packages/crypto/test/providers/CmlEd25519Provider.test.ts Outdated Show resolved Hide resolved
@rhyslbw rhyslbw self-requested a review January 31, 2023 08:58
rhyslbw
rhyslbw requested changes Jan 31, 2023
View reviewed changes
Copy link
Member

@rhyslbw rhyslbw left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'd like to see us avoid overloading the provider term, since it has a specific meaning of establishing query or command access to the blockchain. How about we use the term 'lib' here as a substitute?

@AngelCastilloB
Copy link
Member Author

AngelCastilloB commented Jan 31, 2023 •
edited
Loading

I'd like to see us avoid overloading the provider term, since it has a specific meaning of establishing query or command access to the blockchain. How about we use the term 'lib' here as a substitute?

I thought the use of the provider term was because we are implementing the strategy design pattern there 😅, I will then rename the interface to:

Bip32Ed25519Lib

And the concrete implementations to:

CmlBip32Ed25519Lib
SodiumBip32Ed25519Lib

Following both this feedback and the one from @mkazlauskas here #589 (comment)

Is this acceptable?

@AngelCastilloB AngelCastilloB force-pushed the feat/crypto-library-interface-and-key-management-refactor branch 3 times, most recently from c969a3d to c831ca1 Compare January 31, 2023 23:51
@rhyslbw
Copy link
Member

rhyslbw commented Feb 1, 2023

Let's drop Lib too, and go with:

  • Bip32Ed25519
  • CmlBip32Ed25519
  • SodiumBip32Ed25519

@AngelCastilloB AngelCastilloB force-pushed the feat/crypto-library-interface-and-key-management-refactor branch from c831ca1 to a7381a6 Compare February 1, 2023 18:39
@AngelCastilloB
Copy link
Member Author

Let's drop Lib too, and go with:

  • Bip32Ed25519
  • CmlBip32Ed25519
  • SodiumBip32Ed25519

done here 2c334df

@AngelCastilloB AngelCastilloB force-pushed the feat/crypto-library-interface-and-key-management-refactor branch 3 times, most recently from e21e667 to 00f3e8c Compare February 2, 2023 18:08
@AngelCastilloB AngelCastilloB force-pushed the feat/crypto-library-interface-and-key-management-refactor branch 2 times, most recently from 3d7784d to 0c405e0 Compare February 3, 2023 00:49
@AngelCastilloB AngelCastilloB changed the title [WIP] ADP-2574 feat/crypto library interface and key management refactor ADP-2574 feat/crypto library interface and key management refactor Feb 3, 2023
@AngelCastilloB AngelCastilloB force-pushed the feat/crypto-library-interface-and-key-management-refactor branch from 0c405e0 to 4929194 Compare February 3, 2023 00:56
@AngelCastilloB AngelCastilloB requested review from rhyslbw, mkazlauskas and a team February 3, 2023 01:01
@AngelCastilloB AngelCastilloB force-pushed the feat/crypto-library-interface-and-key-management-refactor branch from 4929194 to 8d8472d Compare February 3, 2023 01:18
mkazlauskas
mkazlauskas requested changes Feb 3, 2023
View reviewed changes
packages/crypto/package.json Outdated Show resolved Hide resolved
packages/governance/test/cip36.test.ts Show resolved Hide resolved
mkazlauskas
mkazlauskas previously approved these changes Feb 3, 2023
View reviewed changes
Copy link
Member

@mkazlauskas mkazlauskas left a comment

Choose a reason for hiding this comment

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

Nice work! 🚀

@AngelCastilloB
feat(crypto): added crypto package to the SDK
030753d
@AngelCastilloB
refactor!: refactor the SDK to use the new crypto package
3b41320 
BREAKING CHANGE:
 - Bip32PublicKey removed from core and replaced by the Bip32PublicKeyHex type from the crypto package.
 - Bip32PrivateKey removed from core and replaced by the Bip32PrivateKeyHex type from the crypto package.
 - Ed25519PublicKey removed from core and replaced by the Ed25519PublicKeyHex type from the crypto package.
 - Ed25519PrivateKey removed from core and replaced by the Ed25519PrivateKeyHex type from the crypto package.
 - Ed25519KeyHash removed from core and replaced by the Ed25519KeyHashHex type from the the crypto package.
 - Ed25519Signature removed from core and replaced by the Ed25519SignatureHex type from the crypto package.
 - Hash32ByteBase16 removed from core and replaced by the Hash32ByteBase16 type from the crypto package.
 - Hash28ByteBase16 removed from core and replaced by the Hash28ByteBase16 type from the crypto package.
 - The KeyAgent interface now has a new field bip32Ed25519.
 - The KeyAgentBase class and all its derived classes (InMemoryKeyAgent, LedgerKeyAgent and TrezorKeyAgent)  must now be provided with a Bip32Ed25519 implementation on their constructors.
 - Bip32Path type was removed from the key-management package and replaced by the Bip32Path from the crypto package.
@AngelCastilloB
test(governance): direct CML call replaced by a call to Bip32Ed25519 …
43d9cdf 
…interface
@AngelCastilloB
fix(governance)!: remove double hardening from CIP-36 derivation indices
9b16323 
BREAKING CHANGE:

Remove hardening from the definition of CIP-36 indices Purpose (1694) and CoinType (1815)
@AngelCastilloB AngelCastilloB force-pushed the feat/crypto-library-interface-and-key-management-refactor branch from b4e18c0 to 9b16323 Compare February 3, 2023 16:16
rhyslbw
rhyslbw approved these changes Feb 3, 2023
View reviewed changes
Copy link
Member

@rhyslbw rhyslbw left a comment

Choose a reason for hiding this comment

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

Awesome work here @AngelCastilloB 🤩

@AngelCastilloB AngelCastilloB merged commit e910b23 into master Feb 6, 2023
@AngelCastilloB AngelCastilloB deleted the feat/crypto-library-interface-and-key-management-refactor branch February 6, 2023 12:26
@AngelCastilloB AngelCastilloB mentioned this pull request Feb 7, 2023
Merged
YorikSar added a commit that referenced this pull request Feb 20, 2023
@YorikSar
build: Add crypto package to the Nix build
df3aa1a 
It was missing since this PR landed:
#589
@YorikSar YorikSar mentioned this pull request Feb 20, 2023
Merged
rhyslbw pushed a commit that referenced this pull request Feb 22, 2023
@YorikSar @rhyslbw
build: Add crypto package to the Nix build
2586b2e 
It was missing since this PR landed:
#589
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rhyslbw rhyslbw rhyslbw approved these changes

@mkazlauskas mkazlauskas mkazlauskas approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@AngelCastilloB @rhyslbw @mkazlauskas