-
Notifications
You must be signed in to change notification settings - Fork 25
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
remove networkSeed input from setKeys / genKeyfile flows #85
Conversation
… if they haven’t been set in this session - Remove networkSeed inputs on genKeyfile/setKeys pages
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 actually feels pretty good, flow-wise. I'd say this is a good solution. Left some comments with minor change requests though!
Additionally, we may want the "Ok ->" from the transaction confirmation screen to be "Download keyfile ->" instead, if that's doable with the forward-only router we have now/are getting.
slow confirm?
src/Bridge.js
Outdated
// import * as azimuth from 'azimuth-js' | ||
// import { CONTRACT_ADDRESSES } from './lib/contracts' | ||
// import { walletFromMnemonic } from './lib/wallet' | ||
|
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.
Eww, commented code! (:
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.
Ah, yeah. This was just a convenience for the commented-out development mode workflow. Removed it.
@@ -92,20 +99,22 @@ class Bridge extends React.Component { | |||
// | |||
// this.setState({ | |||
// routeCrumbs: Stack([ | |||
// ROUTE_NAMES.CREATE_GALAXY, | |||
// // ROUTE_NAMES.CREATE_GALAXY, | |||
// // ROUTE_NAMES.SHIP, |
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 don't see anything about this in your commit messages, but I think these (and the couple lines below) are just dev setup tweaks?
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.
Yep; hopefully that's okay. There were a couple issues with the flow so I just updated it.
src/views/GenKeyfile.js
Outdated
networkSeedWarning = ( | ||
<P> | ||
<b>Warning: </b> | ||
{ "No network seed detected. To generate a keyfile, you must reset your networking keys." } |
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 should also inform the user of the alternative: getting into a state where we can detect networking seed. So, "If you've previously set networking keys while logged in with a master ticket or management mnemonic, logging in with that again might allow you to find the current networking seed." or something that's worded perhaps a little bit better.
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.
How about "Try logging in with your master ticket or management mneumonic if you want to keep your current networking keys" ?
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.
Problem is it would only work if the current keys were actually set using those.
Maybe "Try logging in with your master ticket or management mnemonic. Alternatively, reset your keys."? All of this feels pretty aggressive if the user isn't aware that this is "quick and easy", but maybe it's good if they just find out for themselves?
@@ -252,6 +268,8 @@ class Bridge extends React.Component { | |||
addToPointCache={ this.addToPointCache } | |||
pointCursor={ pointCursor } | |||
pointCache={ pointCache } | |||
networkSeedCache= { networkSeedCache } | |||
setNetworkSeedCache= { this.setNetworkSeedCache } |
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.
React Q, to confirm my understanding: putting these as properties into the View
is what makes these accessible by other components?
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.
Right. If you have some component Foo
, then <Foo foo=1 bar=2 />
is that component with the foo
and bar
properties set to 1 and 2 respectively. In src/Bridge.js
, View
is actually just whatever component is currently being pointed at by the router, so it gets passed all the properties you see listed there.
src/views/SetKeys.js
Outdated
<P> | ||
{ | ||
`If you've authenticated with a master ticket or management proxy | ||
mnemonic, a seed will be generated for you automatically.` | ||
`Once the transaction is sent, please generate your Arvo keyfile immediately.` |
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 sentence needs more gravity. We should probably put it in one of those orange warning boxes, like the one we present when you try to configure keys for the very first time.
Might also want to add a brief description why. "Your network seed is not deterministic. Once the transaction ..."
Also I guess it's a little silly that on the "download keyfile" page, if literally the only thing you can do is click "generate", you still need to click that manually. We should just remove that button and auto-generate when we can. |
@@ -122,22 +124,34 @@ class SetKeys extends React.Component { | |||
}) | |||
} | |||
|
|||
randomHex(len) { |
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 web3 has a function for this. Supposedly "cryptographically strong". We should use that instead of rolling our own.
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.
So I tried this out, and it turns out the function is pretty borked; it doesn't generate strings of consistent length >.<. How important is 'cryptographic strength' here do you think?
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.
sigh
Just slap a //TODO use web3.utils.randomHex, see web3.js#1490
in there and call it a day. web3.js is such a shitshow rn. ):
Yeah, this page is really sparse, and linking to it after setting your keys feels overly complicated. Could we just move this to the point detail page? At the bottom, after "Public Keys", we just have a section called Arvo Keyfile. If your keys have been set, it just presents you with a download button. If not, it directs you to set your keys. What do you think? |
src/views/SetKeys.js
Outdated
<h3 className={'mb-2'}>{'Warning'}</h3> | ||
{ | ||
`Your network seed could not be derived automatically. We've | ||
generated a random one for you, but you must download your Arvo |
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.
generated a random one for you, but you must download your Arvo | |
generated a random one for you, so you must download your Arvo |
That might be better, flow-wise, as long as it doesn't need to be scrolled into view. @jyng thoughts? |
Ok, just pushed the following improvements:
Lmk if you see anything missing! |
@Fang- Whoops, forgot to add the conditional in. Alright, it's fixed and merged. |
This PR makes several changes to the setKeys / generateKeyfile flows:
There's one sticking point with the above flow; if the non-urbit-wallet user refreshes the page between setting network keys and generating their keyfile, the network seed they use is lost. It's fairly punishing if a user accidentally refreshes between steps. Some options we have include:
A) adding the networkSeed to localStorage,
B) making the networkSeed generation deterministic (like urbit wallet case)
C) download the keyfile automatically when new keys are set (combining the two actions into one)
For now Mark and I decided to just inform the user to download their keyfile immediately, but curious if others (@iamwillkim ?) have input here.