Skip to content
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

Merged
merged 8 commits into from
Mar 11, 2021

Conversation

moshthepitt
Copy link
Contributor

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)

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
src/components/helpers/tests/import.test.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@armaniferrante armaniferrante left a 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",
Copy link
Contributor

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.

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link

@evanpipta evanpipta Feb 5, 2021

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.

Copy link
Contributor Author

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.

src/utils/utils.ts Outdated Show resolved Hide resolved
@armaniferrante armaniferrante merged commit c1fb770 into project-serum:master Mar 11, 2021
@moshthepitt moshthepitt deleted the 74-followup branch April 2, 2021 11:13
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