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

remove networkSeed input from setKeys / genKeyfile flows #85

Merged
merged 13 commits into from
Apr 1, 2019
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 21 additions & 3 deletions src/Bridge.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,11 @@ import { NETWORK_NAMES } from './lib/network'
import { WALLET_NAMES, DEFAULT_HD_PATH } from './lib/wallet'
import { BRIDGE_ERROR } from './lib/error'

// import Web3 from 'web3'
// import * as azimuth from 'azimuth-js'
// import { CONTRACT_ADDRESSES } from './lib/contracts'
// import { walletFromMnemonic } from './lib/wallet'

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eww, commented code! (:

Copy link
Contributor Author

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.

import './style/index.css'

class Bridge extends React.Component {
Expand Down Expand Up @@ -51,6 +56,7 @@ class Bridge extends React.Component {
// urbit wallet-related
urbitWallet: Maybe.Nothing(),
authMnemonic: Maybe.Nothing(),
networkSeedCache: null,
// point
pointCursor: Maybe.Nothing(),
pointCache: {},
Expand All @@ -70,6 +76,7 @@ class Bridge extends React.Component {
this.setAuthMnemonic = this.setAuthMnemonic.bind(this)
this.setPointCursor = this.setPointCursor.bind(this)
this.setTxnHashCursor = this.setTxnHashCursor.bind(this)
this.setNetworkSeedCache = this.setNetworkSeedCache.bind(this)
this.addToPointCache = this.addToPointCache.bind(this)
}

Expand All @@ -82,7 +89,7 @@ class Bridge extends React.Component {

// if (process.env.NODE_ENV === 'development') {
//
// const socket = 'wss://localhost:8545'
// const socket = 'ws://localhost:8545'
// const provider = new Web3.providers.WebsocketProvider(socket)
// const web3 = new Web3(provider)
// const contracts = azimuth.initContracts(web3, CONTRACT_ADDRESSES.DEV)
Expand All @@ -92,20 +99,22 @@ class Bridge extends React.Component {
//
// this.setState({
// routeCrumbs: Stack([
// ROUTE_NAMES.CREATE_GALAXY,
// // ROUTE_NAMES.CREATE_GALAXY,
// // ROUTE_NAMES.SHIP,
Copy link
Member

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?

Copy link
Contributor Author

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.

// ROUTE_NAMES.SHIPS,
// // ROUTE_NAMES.MNEMONIC,
// ROUTE_NAMES.WALLET,
// ROUTE_NAMES.NETWORK,
// ROUTE_NAMES.LANDING
// ]),
// networkType: NETWORK_NAMES.LOCAL,
// pointCursor: Maybe.Just(0),
// web3: Maybe.Just(web3),
// contracts: Maybe.Just(contracts),
// walletType: WALLET_NAMES.MNEMONIC,
// wallet: walletFromMnemonic(mnemonic, hdpath),
// urbitWallet: Maybe.Nothing(),
// authMnemonic: Maybe.Nothing()
// authMnemonic: Maybe.Just('benefit crew supreme gesture quantum web media hazard theory mercy wing kitten')
// })
// }
}
Expand Down Expand Up @@ -146,6 +155,12 @@ class Bridge extends React.Component {
}
}

setNetworkSeedCache(networkSeed) {
this.setState({
networkSeedCache: networkSeed
})
}

setNetwork(web3, contracts) {
this.setState({
web3: web3,
Expand Down Expand Up @@ -202,6 +217,7 @@ class Bridge extends React.Component {
wallet,
urbitWallet,
authMnemonic,
networkSeedCache,
pointCursor,
pointCache,
txnHashCursor,
Expand Down Expand Up @@ -252,6 +268,8 @@ class Bridge extends React.Component {
addToPointCache={ this.addToPointCache }
pointCursor={ pointCursor }
pointCache={ pointCache }
networkSeedCache= { networkSeedCache }
setNetworkSeedCache= { this.setNetworkSeedCache }
Copy link
Member

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?

Copy link
Contributor

@jtobin jtobin Mar 25, 2019

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.

// txn
setTxnHashCursor={ this.setTxnHashCursor }
txnHashCursor={ txnHashCursor } />
Expand Down
52 changes: 25 additions & 27 deletions src/views/GenKeyfile.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import React from 'react'
import Maybe from 'folktale/maybe'

import { Button } from '../components/Base'
import { RequiredInput, InnerLabel } from '../components/Base'
Expand Down Expand Up @@ -34,7 +35,12 @@ class GenKeyfile extends React.Component {

async deriveSeed() {
const next = false
const seed = await attemptSeedDerivation(next, this.props)
let seed = await attemptSeedDerivation(next, this.props)

if (seed.getOrElse('') === '' && this.props.networkSeedCache) {
seed = Maybe.Just(this.props.networkSeedCache)
}

this.setState({
networkSeed: seed.getOrElse('')
})
Expand Down Expand Up @@ -101,6 +107,17 @@ class GenKeyfile extends React.Component {
</Button>
: ''

let networkSeedWarning

if (this.state.networkSeed === '') {
networkSeedWarning = (
<P>
<b>Warning: </b>
{ "No network seed detected. To generate a keyfile, you must reset your networking keys." }
Copy link
Member

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.

Copy link
Contributor Author

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" ?

Copy link
Member

@Fang- Fang- Mar 27, 2019

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?

</P>
)
}

return (
<Row>
<Col className={'col-md-8'}>
Expand All @@ -110,33 +127,12 @@ class GenKeyfile extends React.Component {
{ "Generate a private key file for booting this point in Arvo." }
</P>

<P>
{
`Enter a network seed below for generating your key file. Your
network seed must be a string of 64 characters (containing 0-9, A-Z, a-z)`
}
</P>
<P>
{
`If you've authenticated with either a master ticket or a
management proxy mnemonic, a seed will be generated for you
automatically.`
}
</P>

<RequiredInput
className='mono'
prop-size='lg'
prop-format='innerLabel'
autoFocus
value={ networkSeed }
onChange={ this.handleNetworkSeedInput }>
<InnerLabel>{ 'Network seed' }</InnerLabel>
</RequiredInput>
{ networkSeedWarning }

<Button
className={'mt-8'}
color={'blue'}
disabled={this.state.networkSeed === ''}
onClick={
() => {
if (hexRegExp.test(networkSeed)) {
Expand All @@ -149,9 +145,11 @@ class GenKeyfile extends React.Component {
{ 'Generate →' }
</Button>

<Code>
{ keyfile }
</Code>
{ keyfile !== '' &&
<Code className="pb-5">
{ keyfile }
</Code>
}

{ warning }

Expand Down
79 changes: 46 additions & 33 deletions src/views/SetKeys.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@ import { RequiredInput, InnerLabel } from '../components/Base'
import StatelessTransaction from '../components/StatelessTransaction'
import { BRIDGE_ERROR } from '../lib/error'
import { ROUTE_NAMES } from '../lib/router'
import { attemptSeedDerivation } from '../lib/keys'
import { attemptSeedDerivation, genKey } from '../lib/keys'

import saveAs from 'file-saver'

import { WALLET_NAMES } from '../lib/wallet'

Expand Down Expand Up @@ -122,22 +124,34 @@ class SetKeys extends React.Component {
})
}

randomHex(len) {
Copy link
Member

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.

Copy link
Contributor Author

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?

web3/web3.js#1490

Copy link
Member

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. ):

let hex = "";

for (var i = 0; i < len; i++) {
hex = hex + ['0', '1', '2', '3', '4', '5', '6', '7', '8', '9', 'A', 'B',
'C', 'D', 'E', 'F'][Math.floor(Math.random() * 16)]
}

return hex;
}

async deriveSeed() {
const next = true
const seed = await attemptSeedDerivation(next, this.props)
let seed = await attemptSeedDerivation(next, this.props)

if (seed.getOrElse('') === '') {
seed = Maybe.Just(this.randomHex(64));
}

this.setState({
networkSeed: seed.getOrElse('')
})
}



handleNetworkSeedInput(networkSeed) {
this.setState({ networkSeed })
}



handleCreateUnsignedTxn() {
const txn = this.createUnsignedTxn()
this.setState({ txn })
Expand Down Expand Up @@ -222,12 +236,34 @@ class SetKeys extends React.Component {
})
}

downloadKeyfile(networkSeed) {
const { pointCache } = this.props
const { pointCursor } = this.props

const point = pointCursor.matchWith({
Just: (pt) => pt.value,
Nothing: () => {
throw BRIDGE_ERROR.MISSING_POINT
}
})

const pointDetails =
point in pointCache
? pointCache[point]
: (() => { throw BRIDGE_ERROR.MISSING_POINT })()

const revision = parseInt(pointDetails.keyRevisionNumber)
const keyfile = genKey(networkSeed, point, revision)
let blob = new Blob([keyfile], {type:"text/plain;charset=utf-8"});
saveAs(blob, `${ob.patp(point).slice(1)}-${revision}.key`)
}

handleSubmit(){
const { props, state } = this
sendSignedTransaction(props.web3.value, state.stx)
.then(sent => {
props.setNetworkSeedCache(this.state.networkSeed)
// this.downloadKeyfile(this.state.networkSeed)
props.setTxnHashCursor(sent)
props.popRoute()
props.pushRoute(ROUTE_NAMES.SENT_TRANSACTION)
Expand Down Expand Up @@ -299,34 +335,22 @@ class SetKeys extends React.Component {
? props.pointCache[state.point]
: (() => { throw BRIDGE_ERROR.MISSING_POINT })()

const isManagementMnemonic =
this.state.isManagementSeed &&
props.walletType === WALLET_NAMES.MNEMONIC

const isMasterTicket =
props.walletType === WALLET_NAMES.TICKET ||
props.walletType === WALLET_NAMES.SHARD

const networkSeedDisabled = isManagementMnemonic || isMasterTicket;

return (
<Row>
<Col>
<H1>
{ 'Set Network Keys For ' } <code>{ `${ob.patp(state.point)}` }</code>
</H1>

<P>
<P className="mt-10">
{
`Please enter a network seed for generating and setting your public
network authentication and encryption keys. Your network seed
must be a string of 64 characters (containing 0-9, A-Z, a-z).`
`Set new authentication and encryption keys for your Arvo ship.`
}
</P>

<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.`
Copy link
Member

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 ..."

}
</P>

Expand All @@ -340,17 +364,6 @@ class SetKeys extends React.Component {
</Warning>
: <div /> }

<RequiredInput
className='mono'
prop-size='lg'
prop-format='innerLabel'
autoFocus
disabled= { networkSeedDisabled }
value={ state.networkSeed }
onChange={ this.handleNetworkSeedInput }>
<InnerLabel>{ 'Network seed' }</InnerLabel>
</RequiredInput>

<StatelessTransaction
// Upper scope
web3={props.web3}
Expand Down