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

feat(local-signing-manager): add derivationPath for account #33

Merged
merged 8 commits into from
Feb 6, 2023
Merged

feat(local-signing-manager): add derivationPath for account #33

merged 8 commits into from
Feb 6, 2023

Conversation

MehdiRaash
Copy link
Contributor

@MehdiRaash MehdiRaash commented Feb 3, 2023 •
edited

Proposed Changes:

  • account object optionally can have derivationPath, { seed: '...', derivationPath: '//test' }
  • Curve type is now added as the feature of the class
  • Different curve type accept different DerivationPath. sr25519 can accept soft/hard path, while ed25519 only accept hard path.

Example of hard path : //one or //one//one and soft path : //once/two.
In addition, the checking for path is being done by underlining PolkadotJS keyring package.

polymath-eric
polymath-eric requested changes Feb 6, 2023
View reviewed changes
Copy link
Collaborator

@polymath-eric polymath-eric left a comment

Choose a reason for hiding this comment

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

Can you add a test in the .spec file. Ideally one for each type.

packages/local-signing-manager/src/types/index.ts Outdated Show resolved Hide resolved
packages/local-signing-manager/src/lib/local-signing-manager.ts Outdated Show resolved Hide resolved
@polymath-eric
Copy link
Collaborator

You said:

Different curve type accept different DerivationPath. sr25519 can accept soft/hard path, while ed25519 only accept hard path.

I don't see validation for this. At the very least add a doc comment on the derivationPath in the type field

MehdiRaash and others added 2 commits February 6, 2023 11:04
@MehdiRaash @polymath-eric
Update packages/local-signing-manager/src/types/index.ts
919412c 
Co-authored-by: polymath-eric <86971143+polymath-eric@users.noreply.github.com>
@MehdiRaash
feat(local-signing-manager): add test
36e2856
@MehdiRaash
Copy link
Contributor Author

You said:

Different curve type accept different DerivationPath. sr25519 can accept soft/hard path, while ed25519 only accept hard path.

I don't see validation for this. At the very least add a doc comment on the derivationPath in the type field

Validation take place by the underlying polkadot packages.

@MehdiRaash @polymath-eric
Update packages/local-signing-manager/src/types/index.ts
93733bc 
Co-authored-by: polymath-eric <86971143+polymath-eric@users.noreply.github.com>
polymath-eric
polymath-eric approved these changes Feb 6, 2023
View reviewed changes
Copy link
Collaborator

@polymath-eric polymath-eric left a comment

Choose a reason for hiding this comment

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

Thanks adding this. LGTM

@sonarcloud
Copy link

sonarcloud bot commented Feb 6, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@polymath-eric
Copy link
Collaborator

The commit error is from adding the suggestion on the UI, which I forgot husky would complain about.

Merging in anyway since its the only error and it will get squashed out.

@polymath-eric polymath-eric merged commit af131b6 into PolymeshAssociation:main Feb 6, 2023
@polymath-eric polymath-eric deleted the feat/derivation branch February 6, 2023 16:56
prashantasdeveloper pushed a commit that referenced this pull request Feb 6, 2023
@semantic-release-bot
chore(release): 📦 local-signing-manager@1.4.0 [skip ci]
d34e12e 
# [1.4.0](https://github.com/PolymeshAssociation/signing-managers/compare/@polymeshassociation/local-signing-manager@1.3.0...@polymeshassociation/local-signing-manager@1.4.0) (2023-02-06)

### Features

* **local-signing-manager:** add `derivationPath` for account ([#33](#33)) ([af131b6](af131b6))
@prashantasdeveloper
Copy link
Collaborator

🎉 This PR is included in version @polymeshassociation/local-signing-manager@1.4.0 🎉

The release is available on:

@prashantasdeveloper
Copy link
Collaborator

🎉 This PR is included in version @polymeshassociation/signing-manager-types@1.2.2 🎉

The release is available on:

@prashantasdeveloper
Copy link
Collaborator

🎉 This PR is included in version @polymeshassociation/browser-extension-signing-manager@1.1.4 🎉

The release is available on:

@prashantasdeveloper
Copy link
Collaborator

🎉 This PR is included in version @polymeshassociation/hashicorp-vault-signing-manager@1.1.7 🎉

The release is available on:

@prashantasdeveloper
Copy link
Collaborator

🎉 This PR is included in version @polymeshassociation/fireblocks-signing-manager@1.0.4 🎉

The release is available on:

@prashantasdeveloper
Copy link
Collaborator

🎉 This PR is included in version @polymeshassociation/approval-signing-manager@1.0.6 🎉

The release is available on:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@polymath-eric polymath-eric polymath-eric approved these changes

@VictorVicente VictorVicente Awaiting requested review from VictorVicente VictorVicente is a code owner

@prashantasdeveloper prashantasdeveloper Awaiting requested review from prashantasdeveloper prashantasdeveloper is a code owner

@sansan sansan Awaiting requested review from sansan sansan is a code owner

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@MehdiRaash @polymath-eric @prashantasdeveloper