-
Notifications
You must be signed in to change notification settings - Fork 393
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
Allow importing of base58 key format #77
Conversation
This modified version can import private keys formated as: - base58 strings - raw arrays Context: project-serum#74 (comment)
This is a work-around for the known jest bug. See: jestjs/jest#4422
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 is great. Just the one issue w.r.t. the tests being switched to node.
package.json
Outdated
@@ -39,7 +39,7 @@ | |||
"fix:prettier": "prettier \"src/**/*.js\" --write", | |||
"start": "react-scripts start", | |||
"build": "react-scripts build", | |||
"test": "react-scripts test", | |||
"test": "react-scripts test --env=node", |
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 breaks the existing App.test.js
. We should probably either remove that test, your test, or run the node tests separately. I'm in favor of running them separately, so that we can have both types of tests in the future.
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.
Running separately, one with the node argument and one without would be good yeah, I've done that on other projects before.
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.
Could you give me an example of what the two test runs would be targeting generally?
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.
If I understand your question correctly, one would target node and would would target the browser. The App.test.js
test fails when you pass in --env=node
because it can't find the browser's localStorage
variable.
So we can split up "test" into two comands: "test:node"
and "test:browser"
, one with and without the --env=node
flag. The "test"
command can then just invoke both of these.
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.
Thanks @armaniferrante. I was asking about the best way to ensure the two target different files. For instance, if the node test runs against App.test.js and possibly others in the future it gains. On the other hand if the "not node" runs on utils.test.ts and possibly others in the future it will fail.
So I'm looking for ideas on how to generically target these tests other than specifying the individual files in the test command. Hopefully that makes sense
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.
@moshthepitt Now I'm curious, why did you need the --env=node
to begin with? All the code should be running in the browser anyway, thought about @armaniferrante 's comments for a min, and I think it actually makes sense to figure out why this didn't work without that... this is a create-react-app so there's no node outside of development so the tests should not run with --env=node
.
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.
@evanpipta it is a work around a known jest bug as far as Uint8Array
is concerned. Which means that the test I wrote will never pass because the code at this point complains that we do not have a valid input. In the solana web3 repo they solve this problem like this.
Clean up Co-authored-by: Armani Ferrante <armaniferrante@gmail.com>
This PR ensures that importing an account works for both base58 strings as well as the raw array format.
See this comment for more context: #74 (comment)