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

feat(iota-keys): Support the Shimmer coin type for the derivation path validation #1742

Merged
merged 5 commits into from
Aug 13, 2024

Conversation

miker83z
Copy link
Contributor

@miker83z miker83z commented Aug 9, 2024

Description of change

This PR simply allows a derivation path containing the Shimmer coin type (4219) to be validated during the keypair creation process in the Rust SDK.

Links to any relevant issues

Related to #1047

Type of change

  • Enhancement (a non-breaking change which adds functionality)

How the change has been tested

cargo run --bin iota keytool generate ed25519 "m/44'/4219'/0'/0'/0'"

Change checklist

  • I have followed the contribution guidelines for this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that new and existing unit tests pass locally with my changes

@miker83z miker83z added the sc-platform Issues related to the Smart Contract Platform group. label Aug 9, 2024
@miker83z miker83z requested review from valeriyr and Thoralf-M August 9, 2024 17:00
@miker83z miker83z self-assigned this Aug 9, 2024
@miker83z miker83z requested a review from a team as a code owner August 9, 2024 17:00
Copy link
Member

@thibault-martinez thibault-martinez left a comment

Choose a reason for hiding this comment

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

The output looks like this

╭─────────────────┬──────────────────────────────────────────────────────────────────────────────────────╮
│ alias           │                                                                                      │
│ iotaAddress     │  0xc17acc2e56d94ae8331205baf5cdb727f036abd787f295f00739cfa45920ff7d                  │
│ publicBase64Key │  ANBMoej0dIKGh36kkcRIbPmWARUc81WugEJkT7ZO3+W1                                        │
│ keyScheme       │  ed25519                                                                             │
│ flag            │  0                                                                                   │
│ mnemonic        │  connect clutch cool raccoon grass plunge filter fitness three vibrant office occur  │
│ peerId          │  d04ca1e8f4748286877ea491c4486cf99601151cf355ae8042644fb64edfe5b5                    │
╰─────────────────┴──────────────────────────────────────────────────────────────────────────────────────╯

Do you think it could be confusing that it never mentions the coin type?

@miker83z
Copy link
Contributor Author

The output looks like this

╭─────────────────┬──────────────────────────────────────────────────────────────────────────────────────╮
│ alias           │                                                                                      │
│ iotaAddress     │  0xc17acc2e56d94ae8331205baf5cdb727f036abd787f295f00739cfa45920ff7d                  │
│ publicBase64Key │  ANBMoej0dIKGh36kkcRIbPmWARUc81WugEJkT7ZO3+W1                                        │
│ keyScheme       │  ed25519                                                                             │
│ flag            │  0                                                                                   │
│ mnemonic        │  connect clutch cool raccoon grass plunge filter fitness three vibrant office occur  │
│ peerId          │  d04ca1e8f4748286877ea491c4486cf99601151cf355ae8042644fb64edfe5b5                    │
╰─────────────────┴──────────────────────────────────────────────────────────────────────────────────────╯

Do you think it could be confusing that it never mentions the coin type?
@thibault-martinez

Shall we open an issue for it?
In principle, on the Rust side, having a diferent coin type shouldn't be considered a feature but more a necessity. Meaning that the user shouldn't be encouraged to use the Shimmer type. Following this reasoning, maybe the only change could be to change the iotaAddress in the column to shimmerAddress when 4219 is used for the derivation? (is it feasible though? maybe it needs additional data to know the coin type in the keystore file...)

@thibault-martinez
Copy link
Member

The output looks like this

╭─────────────────┬──────────────────────────────────────────────────────────────────────────────────────╮
│ alias           │                                                                                      │
│ iotaAddress     │  0xc17acc2e56d94ae8331205baf5cdb727f036abd787f295f00739cfa45920ff7d                  │
│ publicBase64Key │  ANBMoej0dIKGh36kkcRIbPmWARUc81WugEJkT7ZO3+W1                                        │
│ keyScheme       │  ed25519                                                                             │
│ flag            │  0                                                                                   │
│ mnemonic        │  connect clutch cool raccoon grass plunge filter fitness three vibrant office occur  │
│ peerId          │  d04ca1e8f4748286877ea491c4486cf99601151cf355ae8042644fb64edfe5b5                    │
╰─────────────────┴──────────────────────────────────────────────────────────────────────────────────────╯

Do you think it could be confusing that it never mentions the coin type?
@thibault-martinez

Shall we open an issue for it? In principle, on the Rust side, having a diferent coin type shouldn't be considered a feature but more a necessity. Meaning that the user shouldn't be encouraged to use the Shimmer type. Following this reasoning, maybe the only change could be to change the iotaAddress in the column to shimmerAddress when 4219 is used for the derivation? (is it feasible though? maybe it needs additional data to know the coin type in the keystore file...)

I've opened #1744, let's reassess later, not extremely important.
But since iotaAddress comes from a struct field, it's not straightforward to change it.

@thibault-martinez thibault-martinez merged commit b6ce1cc into develop Aug 13, 2024
32 of 35 checks passed
@thibault-martinez thibault-martinez deleted the sc-platform/validate-shimmer-coin-type branch August 13, 2024 09:08
bingyanglin pushed a commit that referenced this pull request Aug 19, 2024
…h validation (#1742)

* feat(iota-keys): support Shimmer coin type for derivation path validation

* fix(iota-keys): comment
alexsporn pushed a commit that referenced this pull request Sep 6, 2024
…h validation (#1742)

* feat(iota-keys): support Shimmer coin type for derivation path validation

* fix(iota-keys): comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sc-platform Issues related to the Smart Contract Platform group.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants