-
Notifications
You must be signed in to change notification settings - Fork 11.3k
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: unify import/export private key format #15415
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
4 Ignored Deployments
|
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.
After having another look, it seems ExportedKeyPair
and Keypair.export
are not necessary anymore, so I opened this PR to deprecate them.
@@ -0,0 +1,5 @@ | |||
--- | |||
'@mysten/sui.js': major |
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.
not sure if we do major
updates for the sdk - noting for @hayes-mysten or @Jordan-Mysten to confirm
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.
yes everything should be a minor until we get to 1.0
@@ -135,6 +135,7 @@ | |||
"@sentry/browser": "^7.61.0", | |||
"@tanstack/react-query": "^5.0.0", | |||
"@tanstack/react-query-persist-client": "^4.29.25", | |||
"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.
Looks like this might not be needed anymore?
return keypair; | ||
} | ||
|
||
export function fromExportedKeypair(keypair: ExportedKeypair, legacySupport = false): Keypair { |
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.
When will we ever drop legacy support? I'm not sure it makes sense for this to be configurable?
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.
This was added for wallet storage backwards compatibility, cc @pchrysochoidis
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.
We will need to run a storage migration to get rid of this. Although I've this PR #15833 that changes it a bit. We should review/merge that one first.
export function fromExportedKeypair(keypair: ExportedKeypair): Keypair { | ||
const secretKey = fromB64(keypair.privateKey); | ||
export function validateExportedKeypair(keypair: ExportedKeypair): ExportedKeypair { | ||
const _kp = decodeSuiPrivateKey(keypair.privateKey); |
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.
You can just call the function and not grab a return value.
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.
I removed it in #15833
try { | ||
privateKeyBytes = hexToBytes(hexValue); | ||
decodeSuiPrivateKey(privateKey); |
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.
Given it's going to take a while for wallets to update, we probably don't want to ship with this, and should be able to detect legacy formats and upgrade them seamlessly.
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.
allowing both hex and bech32 in the current impl
Co-authored-by: stefan-mysten <135084671+stefan-mysten@users.noreply.github.com>
* this allows supporting keys that are stored in base64 format already in the wallet storage
## Description followup to #15415 ## Test Plan How did you test the new or updated feature? --- If your changes are not user-facing and do not break anything, you can skip the following section. Otherwise, please briefly describe what has changed under the Release Notes section. ### Type of Change (Check all that apply) - [ ] protocol change - [ ] user-visible impact - [ ] breaking change for a client SDKs - [ ] breaking change for FNs (FN binary must upgrade) - [ ] breaking change for validators or node operators (must upgrade binaries) - [ ] breaking change for on-chain data layout - [ ] necessitate either a data wipe or data migration ### Release notes
Description
SIP: sui-foundation/sips#15
CLI changes
sui keytool convert
: this converts hex, base64 to bech32sui keytool import
: can only import as bech32sui keytoo export
: new, export as bech32SDK changes
Wallet changes
Export UI supports Bech32 only.
Import UI supports both Hex and Bech32. Hex importing shows warning for deprecation for March 1, 2024.
Test Plan
unit test
CLI scenario
If your changes are not user-facing and not a breaking change, you can skip the following section. Otherwise, please indicate what changed, and then add to the Release Notes section as highlighted during the release process.
Type of Change (Check all that apply)
Release notes
Sui Keystore CLI no longer supports an import of a private key as a 32-byte
hex
string. It now only supports import or export Bech32-string encoded 33-byteflag || private_key
starting withsuiprivkey
. See usage forsui keytool convert -h
if you want to see all formatted private keys. Seesui keytool export -h
if you need to export a private key in bech32 format. This matches the import and export private key format in Sui Wallet and SDK. See SIP for more standard details.