Skip to content

Conversation

@ethankonk
Copy link
Contributor

No description provided.

@socket-security
Copy link

socket-security bot commented Jan 5, 2026

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Addedbech32@​2.0.01001008184100

View full report

@ethankonk ethankonk force-pushed the ethan/wif-bech32-pk-import-export-2 branch 3 times, most recently from 9a7c8bd to 0bcb477 Compare January 6, 2026 17:29
it("encodes sui bech32 private key correctly", async () => {
const keySui =
"suiprivkey1qpj5xd9396rxsu7h45tzccalhuf95e4pygls3ps9txszn9ywpwsnznaeq0l";
const { _, words} = TKHQ.decodeBech32(keySui);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const { _, words} = TKHQ.decodeBech32(keySui);
const { words} = TKHQ.decodeBech32(keySui);

?

expect(encodedKey).toEqual(keySol);
});

it("encodes bitcoin WIF private key correctly", async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

"dependencies": {
"@hpke/core": "1.7.5"
"@hpke/core": "1.7.5",
"bech32": "^2.0.0"
Copy link
Contributor

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

@ethankonk ethankonk force-pushed the ethan/wif-bech32-pk-import-export-2 branch from 0bcb477 to c4cc20b Compare January 7, 2026 15:21
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
Copy link
Contributor

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?

Copy link
Contributor Author

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.

andrewkmin
andrewkmin previously approved these changes Jan 7, 2026
@ethankonk ethankonk force-pushed the ethan/wif-bech32-pk-import-export-2 branch from c4cc20b to bfb5d72 Compare January 7, 2026 15:56
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.
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor Author

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!

Copy link
Contributor Author

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)

Copy link
Contributor

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.

Copy link
Contributor Author

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


if (prefix !== "suiprivkey") {
throw new Error(
`invalid SUI private key HRP: expoected "suiprivkey", got "${prefix}"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`invalid SUI private key HRP: expoected "suiprivkey", got "${prefix}"`
`invalid SUI private key human-readable part (HRP): expected "suiprivkey"`

Copy link
Contributor

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.

it("decodes sui bech32 private key correctly", async () => {
const keySuiBech32 =
"suiprivkey1qpj5xd9396rxsu7h45tzccalhuf95e4pygls3ps9txszn9ywpwsnznaeq0l";
const { _, words } = bech32.decode(keySuiBech32);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const { _, words } = bech32.decode(keySuiBech32);
const { words } = bech32.decode(keySuiBech32);

expect(encodedKey).toEqual(keyWif);
});

it("encodes sui bech32 private key correctly", async () => {
Copy link
Contributor

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

}

/**
* Decodes a base58-check-encoded string into a buffer
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Decodes a base58-check-encoded string into a buffer
* Encodes a base58-check-encoded string into a buffer
``` ?

);
}

const version = 0x80; // mainnet version byte TODO: support testnet? how would we know which to use?
Copy link
Contributor

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 😿

Copy link
Contributor Author

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

@ethankonk ethankonk force-pushed the ethan/wif-bech32-pk-import-export-2 branch 2 times, most recently from 3f79b23 to 0278efc Compare January 8, 2026 16:32
const keyAndFlags = payload.subarray(1);

// 0x80 = mainnet, 0xEF = testnet
if (version !== 0x80 && version !== 0xEF) {
Copy link
Contributor

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?

Copy link
Contributor Author

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?

andrewkmin
andrewkmin previously approved these changes Jan 8, 2026
@ethankonk ethankonk force-pushed the ethan/wif-bech32-pk-import-export-2 branch 5 times, most recently from 7bb2faa to 7d10c10 Compare January 12, 2026 16:18
@ethankonk ethankonk force-pushed the ethan/wif-bech32-pk-import-export-2 branch from 7d10c10 to 20a6009 Compare January 12, 2026 16:21
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.

3 participants