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

Oasis backward changes #1440

Merged
merged 7 commits into from
May 27, 2021
Merged

Conversation

emmanuelm41
Copy link
Contributor

@emmanuelm41 emmanuelm41 commented May 21, 2021

Description

Oasis has requested that any new address should use a SLIP0010 with a derivation path based on BIP32 and 3 values only.
Given that Trustwallet integration is not available yet, the preference is to support this format only in Trustwallet.
This PR adjust wallet-core functionality according to this.

Testing instructions

The CI tests will check the addresses generated with the new curve used.

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • Prefix PR title with [WIP] if necessary.
  • Add tests to cover changes as needed.
  • Update documentation as needed.
  • I have read the guidelines for adding a new blockchain
  • If there is a related Issue, mention it in the description (e.g. Fixes #<issue_number> ).

@optout21
Copy link
Contributor

What's the motivation of this change?
See: #1258 (comment)
Oasis uses ed25519+BIP32 derivation ...

@emmanuelm41
Copy link
Contributor Author

What's the motivation of this change?
See: #1258 (comment)
Oasis uses ed25519+BIP32 derivation ...

I updated the PR description.

@tjanez
Copy link

tjanez commented May 21, 2021

@catenocrypt, I'm speaking on behalf of Oasis.

We've recently (oasisprotocol/oasis-core#3656 (comment)) adopted ADR 0008: Standard Account Key Generation which specifies 3 things:

  • BIP-0039 for mnemonic to seed derivation,
  • SLIP-0010 for hierarchical key derivation,
  • m/44'/474'/x' as the BIP-0032 path, where x is the key/account number with primary key/account having x equal 0.

Since Trustwallet hasn't enabled Oasis support yet, it makes the most sense to revert the key derivation to ADR 0008 standard right away. That way, it will be compatible with all other Oasis wallets in the future.

@emmanuelm41
Copy link
Contributor Author

emmanuelm41 commented May 21, 2021

@catenocrypt @vikmeup I need some help with linux CI tests. I did not change anything related to the compiler, but it is failing. I tried to run CI on master branch in zondax forked repo, and I got the same (even though it is green in master branch on trust wallet). The link is here.

coins.json Show resolved Hide resolved
@tjanez
Copy link

tjanez commented May 21, 2021

FWIW, I've added Trust Wallet Core's test mnemonic (shoot island position soft burden budget tooth cruel issue economy destroy above) to ADR 0008's extended test vectors in oasisprotocol/oasis-core#3960.

And I'm confirming that the address for derivation path m/44'/474'/0' is oasis1qzcpavvmuw280dk0kd4lxjhtpf0u3ll27yf7sqps.

@emmanuelm41 emmanuelm41 marked this pull request as ready for review May 24, 2021 13:32
Copy link

@tjanez tjanez left a comment

Choose a reason for hiding this comment

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

While at it, @emmanuelm41, can you also update the info.url to https://oasisprotocol.org/ and info.clientPublic to https://rosetta.oasis.dev/api/v1?

Should the info.clientDocs point to RPC docs in particular? If not, I would replace it with https://docs.oasis.dev/oasis-core/.

coins.json Outdated Show resolved Hide resolved
@emmanuelm41
Copy link
Contributor Author

@tjanez the values were updated as you indicated. I am not sure if the info.clientDocs should point to RPC docs. Other coins have different docs, so I think we can change it. I did that. We can wait trust wallet to check it.

@emmanuelm41
Copy link
Contributor Author

@catenocrypt @vikmeup I need some help with linux CI tests. I did not change anything related to the compiler, but it is failing. I tried to run CI on master branch in zondax forked repo, and I got the same (even though it is green in master branch on trust wallet). The link is here.

Could you help us?

@hewigovens
Copy link
Contributor

@emmanuelm41 could you please search and delete "ed25519HD" references from code? will take a look at linux CI

@optout21
Copy link
Contributor

Linux CI build error is weird, seems unrelated to changed files in this PR. Investigating.

2021-05-25T02:45:54.1373843Z In file included from In file included from /home/runner/work/wallet-core/wallet-core/build/local/src/gtest/googletest-release-1.10.0/googletest/src/gtest-all.cc:38:
2021-05-25T02:45:54.1378493Z /home/runner/work/wallet-core/wallet-core/build/local/include/gtest/gtest.h:55:10: fatal error: 'cstddef' file not found
2021-05-25T02:45:54.1381511Z #include <cstddef>

@optout21
Copy link
Contributor

It was already failing for last commit on master
https://github.com/trustwallet/wallet-core/runs/2661368297

@hewigovens
Copy link
Contributor

cause: actions/runner-images#3235, will push the fix

@optout21
Copy link
Contributor

Linux CI issue: #1441

@emmanuelm41
Copy link
Contributor Author

@emmanuelm41 could you please search and delete "ed25519HD" references from code? will take a look at linux CI

I think we are done! I just removed everything.

@optout21
Copy link
Contributor

Please merge master (I tried but have no permission). That should fix the Linux CI build.

Copy link
Contributor

@optout21 optout21 left a comment

Choose a reason for hiding this comment

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

Also please remove all occurences of ed25519_hd_info (in bip32.c/h)

trezor-crypto/crypto/bip32.c Show resolved Hide resolved
trezor-crypto/include/TrezorCrypto/curves.h Show resolved Hide resolved
@emmanuelm41
Copy link
Contributor Author

emmanuelm41 commented May 25, 2021

Please merge master (I tried but have no permission). That should fix the Linux CI build.

Done! Thanks! I made the suggested changes. What do you think now?

@emmanuelm41 emmanuelm41 changed the title [WIP] Oasis backward changes Oasis backward changes May 26, 2021
@emmanuelm41
Copy link
Contributor Author

emmanuelm41 commented May 26, 2021

Perfect! So are we ready to merge? Thank you so much!

@hewigovens hewigovens merged commit f8b81d8 into trustwallet:master May 27, 2021
SHH-HaoZhang pushed a commit to monacohq/wallet-core that referenced this pull request Oct 15, 2021
* change curve and derivation path
* update test values to match new coin config
* update oasis coin config
* remove hd curve related code
* remove more stuff related to hd curve
* remove test cases for bip32-hd
cornbread78 pushed a commit to cornbread78/wallet-core that referenced this pull request Dec 22, 2021
* change curve and derivation path
* update test values to match new coin config
* update oasis coin config
* remove hd curve related code
* remove more stuff related to hd curve
* remove test cases for bip32-hd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants