Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

fix: ensure that Keypair.secretKey is a Uint8Array #27700

Merged
merged 1 commit into from
Sep 9, 2022

Conversation

jstarry
Copy link
Contributor

@jstarry jstarry commented Sep 9, 2022

Problem

An unintended side effect of #27435 is that the Keypair.secretKey property is no longer coerced into a Uint8Array and therefore the toString behavior has been changed.

Summary of Changes

Ensure that Keypair.secretKey is always a proper Uint8Array and not a Buffer in disguise

Fixes #27684

@steveluscher
Copy link
Contributor

Wait. Really? Not at a keyboard right now but doesn’t Buffer support the entire Uint8Array interface?

What’s the different toString behavior?

@codecov
Copy link

codecov bot commented Sep 9, 2022

Codecov Report

Merging #27700 (6946062) into master (e779032) will increase coverage by 0.3%.
The diff coverage is n/a.

❗ Current head 6946062 differs from pull request most recent head 2ecba28. Consider uploading reports for the commit 2ecba28 to get more accurate results

@@            Coverage Diff            @@
##           master   #27700     +/-   ##
=========================================
+ Coverage    76.9%    77.2%   +0.3%     
=========================================
  Files          48       55      +7     
  Lines        2505     2888    +383     
  Branches      355      402     +47     
=========================================
+ Hits         1927     2232    +305     
- Misses        448      514     +66     
- Partials      130      142     +12     

@jstarry
Copy link
Contributor Author

jstarry commented Sep 9, 2022

It supports the interface but the toString() behavior is different. Buffer.toString() returns a utf8 encoded string whereas Uint8Array returns a comma separated list of byte values.

@steveluscher
Copy link
Contributor

Oh, I just read #27684.

Sigh. It’s a shame to have to cast and recast values, but here we are!

@jstarry
Copy link
Contributor Author

jstarry commented Sep 9, 2022

Sigh. It’s a shame to have to cast and recast values, but here we are!

Yeah, but fortunately the recast is cheap in this case because it's just creating a view over the same array buffer under the hood!

@steveluscher
Copy link
Contributor

Are we super sure we should change this? Any use case that stringifies the secret key is almost certainly not a good one, right?

@jstarry
Copy link
Contributor Author

jstarry commented Sep 9, 2022

Are we super sure we should change this? Any use case that stringifies the secret key is almost certainly not a good one, right?

Yeah, I was surprised that anyone hit this at all but I don't see any harm in reverting to previous behavior. A utf8 encoded secret is no better or worse than a csv string with the same data

@jstarry
Copy link
Contributor Author

jstarry commented Sep 9, 2022

The test failure is unrelated, I'll patch that up in a follow up

@jstarry jstarry merged commit 080f085 into solana-labs:master Sep 9, 2022
@jstarry jstarry deleted the web3/fix-keypair-secretkey branch September 9, 2022 20:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The secretKey property on the Keypair class from @solana/web3.js behaves differently since 1.54.1
2 participants