-
Notifications
You must be signed in to change notification settings - Fork 1
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
[#9] Support API key #12
Conversation
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.
Nice updates! Will leave the bulk of the review to @n0thingness (and/or others), but one note about API keys and unique URLs.
@n0thingness Please take a look at when you are available, no need to rush. :) |
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.
LGTM 🚀
@daenamkim , please do a review on my additional changes before merging. In the end I pulled out the Mock3 config. It is supremely dangerous to encourage users to store private Ethereum keys in plain text. Especially in the source code. And even more so because in our current setup they will be packaged with the production build.
I think we can revisit this in the future but there has to be a way to make it:
- Clear that it is a test account subject to public exposure and control
- Configurable in a way that it is in a gitignore config file and not in the actual UI code
- Injected by testcafe so that it is not in the actual build (thanks Pierre for reminder about this functionality https://devexpress.github.io/testcafe/documentation/guides/advanced-guides/inject-client-scripts.html)
- (Alternatively we could use a dedicated curvenet server, HSM enabled server, or mock out MultiBaas itself for the tests)
This will be the first interaction with MultiBaas for many new users, perhaps new to blockchain, and we do not want their first action to be exporting private keys insecurely 😉
Anyway, it was a good idea to add tests 👍 I kept the get balance test and marked the mint test as 'skip'.
Thanks for upgrading us to the API from the username / password and thanks for being patient about the review! 🍦
Resoves #9