-
Notifications
You must be signed in to change notification settings - Fork 2
Added Bitcoin WIF & Sui Bech32 private key import/export #109
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
base: main
Are you sure you want to change the base?
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
9a7c8bd to
0bcb477
Compare
export/index.test.js
Outdated
| it("encodes sui bech32 private key correctly", async () => { | ||
| const keySui = | ||
| "suiprivkey1qpj5xd9396rxsu7h45tzccalhuf95e4pygls3ps9txszn9ywpwsnznaeq0l"; | ||
| const { _, words} = TKHQ.decodeBech32(keySui); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| const { _, words} = TKHQ.decodeBech32(keySui); | |
| const { words} = TKHQ.decodeBech32(keySui); |
?
| expect(encodedKey).toEqual(keySol); | ||
| }); | ||
|
|
||
| it("encodes bitcoin WIF private key correctly", async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import/package.json
Outdated
| "dependencies": { | ||
| "@hpke/core": "1.7.5" | ||
| "@hpke/core": "1.7.5", | ||
| "bech32": "^2.0.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's hardpin this version
0bcb477 to
c4cc20b
Compare
import/src/turnkey-core.js
Outdated
| const schemeFlag = bytes[0]; | ||
| const privkey = bytes.slice(1); | ||
|
|
||
| // TODO: schemeFlag = 0 is Ed25519; figure out if we need to support dynamic curve detection or something |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did we end up wanting to do anything with schemeFlag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah good catch, forgot about this, I don't think we need to do anything with it since it seems we only support ED25519 keys for SUI anywase, but will leave a modified version of this comment in-case anyone loops back on this in the future.
c4cc20b to
bfb5d72
Compare
import/src/turnkey-core.js
Outdated
| const schemeFlag = bytes[0]; | ||
| const privkey = bytes.slice(1); | ||
|
|
||
| // schemeFlag = 0 is Ed25519; We currently only support Ed25519 keys for SUI so we do not need to check further. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to ensure that schemeFlag is indeed 0? or can it be fully disregarded?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, thats actually a good point, will look into whether this actually matters or not real quick
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok yep turns out the scheme flag is important since the derived addresses would not match the "expected" addresses from the private key imported (since we'd only be deriving ED25519 addresses rather than the expected curve). Will update this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although, couldn't we also support SECP256k1 SUI keys in theory? Might test that out as well but that would also open up a new can of worms on the export side... (choosing the correct scheme flag to encode with)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although, couldn't we also support SECP256k1 SUI keys in theory? Might test that out as well but that would also open up a new can of worms on the export side... (choosing the correct scheme flag to encode with)
yeah that's a good observation! We technically could, but we don't on the mono side at the moment, and I'm not sure we'll need to anytime soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok we good, just tried and we indeed only support ED25519 SUI address derivation
import/src/turnkey-core.js
Outdated
|
|
||
| if (prefix !== "suiprivkey") { | ||
| throw new Error( | ||
| `invalid SUI private key HRP: expoected "suiprivkey", got "${prefix}"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| `invalid SUI private key HRP: expoected "suiprivkey", got "${prefix}"` | |
| `invalid SUI private key human-readable part (HRP): expected "suiprivkey"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's refrain from returning prefix in the error message, just out of an abundance of caution, given it's sensitive material.
import/index.test.js
Outdated
| it("decodes sui bech32 private key correctly", async () => { | ||
| const keySuiBech32 = | ||
| "suiprivkey1qpj5xd9396rxsu7h45tzccalhuf95e4pygls3ps9txszn9ywpwsnznaeq0l"; | ||
| const { _, words } = bech32.decode(keySuiBech32); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| const { _, words } = bech32.decode(keySuiBech32); | |
| const { words } = bech32.decode(keySuiBech32); |
| expect(encodedKey).toEqual(keyWif); | ||
| }); | ||
|
|
||
| it("encodes sui bech32 private key correctly", async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add some negative test cases (failures) as well? for both this test file as well as import's
export/index.template.html
Outdated
| } | ||
|
|
||
| /** | ||
| * Decodes a base58-check-encoded string into a buffer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * Decodes a base58-check-encoded string into a buffer | |
| * Encodes a base58-check-encoded string into a buffer | |
| ``` ? |
export/index.template.html
Outdated
| ); | ||
| } | ||
|
|
||
| const version = 0x80; // mainnet version byte TODO: support testnet? how would we know which to use? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, I think this is also to be addressed. Have you been able to test exporting mainnet vs testnet? I figure we'd want to be able to support both; if necessary, we could introduce another case: BITCOIN_MAINNET_WIF, BITCOIN_TESTNET_WIF 😿
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahhh yes that would be an issue.. will fix that up as well. I think supporting another case would be the cleanest way of doing it. Can't really think of another way atm
3f79b23 to
0278efc
Compare
import/src/turnkey-core.js
Outdated
| const keyAndFlags = payload.subarray(1); | ||
|
|
||
| // 0x80 = mainnet, 0xEF = testnet | ||
| if (version !== 0x80 && version !== 0xEF) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are your thoughts on ensuring BITCOIN_MAINNET_WIF aligns with 0x80 and BITCOIN_TESTNET_WIF with 0xEF?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As in link a reference to double check? Or kind of always ensure we use the right values? I don't think the standard should ever change right?
7bb2faa to
7d10c10
Compare
7d10c10 to
20a6009
Compare
No description provided.