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.

Sign up for GitHub

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

Add transaction status indicator to SentTransaction #99

Merged
merged 7 commits into from
Apr 16, 2019
Merged

Add transaction status indicator to SentTransaction #99

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
58ac642
Add transaction status indicator to SentTransaction
pkova Apr 8, 2019
31aa349
Animate pending dots
pkova Apr 16, 2019
765deb1
Dont put divs inside p
pkova Apr 16, 2019
bf869de
Give status an esanchor their own paragraphs
pkova Apr 16, 2019
e2a43bb
Fix bug in interval clearing on component unmount
pkova Apr 16, 2019
d67f269
Remove dangling p element
pkova Apr 16, 2019
8de9bc3
Return nothing instead of empty div when esvisible == false
pkova Apr 16, 2019
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
15 changes: 13 additions & 2 deletions src/bridge/Bridge.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@ class Bridge extends React.Component {
pointCursor: Maybe.Nothing(),
pointCache: {},
// txn
txnHashCursor: Maybe.Nothing()
txnHashCursor: Maybe.Nothing(),
txnConfirmations: {}
}

this.pushRoute = this.pushRoute.bind(this)
Expand All @@ -95,6 +96,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.setTxnConfirmations = this.setTxnConfirmations.bind(this)
this.setNetworkSeedCache = this.setNetworkSeedCache.bind(this)
this.addToPointCache = this.addToPointCache.bind(this)
}
Expand Down Expand Up @@ -234,6 +236,12 @@ class Bridge extends React.Component {
this.setState({ txnHashCursor })
}

setTxnConfirmations(txnHash, txnConfirmations) {
this.setState((prevState, _) =>
({txnConfirmations: { ...prevState.txnConfirmations,
[txnHash]: txnConfirmations}}))
}

Fang- marked this conversation as resolved.
Show resolved Hide resolved
render() {
const {
routeCrumbs,
Expand All @@ -248,6 +256,7 @@ class Bridge extends React.Component {
pointCursor,
pointCache,
txnHashCursor,
txnConfirmations,
web3,
contracts
} = this.state
Expand Down Expand Up @@ -300,7 +309,9 @@ class Bridge extends React.Component {
setNetworkSeedCache= { this.setNetworkSeedCache }
// txn
setTxnHashCursor={ this.setTxnHashCursor }
txnHashCursor={ txnHashCursor } />
txnHashCursor={ txnHashCursor }
setTxnConfirmations={ this.setTxnConfirmations }
txnConfirmations={ txnConfirmations } />
Copy link
Member

Choose a reason for hiding this comment

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

@c-johnson I continue to feel highly uncomfortable with the dumping of state into Bridge.js for the sole purpose of passing it into new/different screens. In a lot of these cases, especially ones like these where only a single, temporary screen needs the data, it seems much more desirable to pass it in via the pushRoute data argument.

How would you feel about standardizing on a pattern for that? Would it be possible to do that in such a way that data is always explicitly passed forward, instead of stored in pseudo-global state? I'm trying to steer towards a slightly more functional style here. @jtobin may also have thoughts.

Copy link
Contributor

Choose a reason for hiding this comment

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

In general I think that's a great idea. I would just try to ensure routeData maintains some sort of sane/predictable structure -- I think using a dynamically-typed blob consisting of whatever the downstream component needs doesn't necessarily scale well, for example.

Copy link
Contributor

@c-johnson c-johnson Apr 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

A couple things I can think of;

  • We could bundle the routeData with the actual routeCrumb entry, so it's at least coupled to its view and not a free-floating variable:
    in Bridge.js#pushRoute

    this.setState((state, _) => ({
  routeCrumbs: state.routeCrumbs.push({
    view: symbol,
    data: routeData
  })
}));

  • Adding propType validators on views that take custom route data to shore up the structure somewhat:
    in Bridge.js#pushRoute

    SentTransaction.propTypes = {
  routeData:  PropTypes.shape({
    promptKeyfile: PropTypes.bool
  })
}

I can't think of many other ways to isolate route data from global state, since all pushRoute has access to is updating Bridge.js's state variable. But definitely open for other suggestions if you think of anything.


<div className={'push'} />

Expand Down
2 changes: 1 addition & 1 deletion src/bridge/components/StatelessTransaction.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ class StatelessTransaction extends React.Component {

submit(){
const { props, state } = this
sendSignedTransaction(props.web3.value, state.stx)
sendSignedTransaction(props.web3.value, state.stx, props.setTxnConfirmations)
Copy link
Contributor

Choose a reason for hiding this comment

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

I just noticed that web3 isn't being unwrapped here. I.e., web3 could be Nothing, such that web3.value would be undefined. It should be unwrapped in the standard fashion; something like:

const web3 = props.web3.matchWith({
  Just: w3 => w3.value,
  Nothing: _ => { throw BRIDGE_ERROR.MISSING_WEB3 }
})

sendSignedTransaction(web3, ...)

That folktale lets one just wing it on the value property is pretty lousy, but alas, such is the nature of JS.

(This isn't actually related to your change @pkova -- I just happened to spot it while scrolling through the diff.)

Copy link
Member

Choose a reason for hiding this comment

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

Is it not ok to use .value directly if pre-condition logic (ie whatever you need to do to make submit() accessible) already guarantees/checks for Just? (Of course, figuring that out as the reader isn't always trivial, but "trust the author"? (^: )

Copy link
Contributor

Choose a reason for hiding this comment

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

The reason you want to be pedantic and do it literally everywhere is that it's easy to change the code (perhaps in a refactoring), remove the precondition, but forget to change the yolo .value attempt. It's a recipe for introducing annoying bugs eventually.

Better would probably be to do the unwrapping once, in one place, consistently. This is what I was thinking when I mentioned unwrapping all the necessary props in the constructor, or at least all in one place, and then to use the unwrapped values elsewhere in the component. Maybe something like:

class Foo extends React.Component {
  constructor(props) {
    super(props)

    this.web3 = props.web3.matchWith({
      Nothing: _ => throw { BRIDGE_ERROR.MISSING_WEB3 },
      Just: w3 => w3.value
    })
  }

  render() {
    const web3 = this.web3

    return(
      <div>{ web3 }</div>
    )
  }
}

Copy link
Member

Choose a reason for hiding this comment

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

Definitely in favor of unwrapping once in the constructor, yes. I'll put that on my informal refactoring list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't like Maybe. Using it has the upside of 'forcing you to check' for missing values, but the tradeoff is that changes that should be non-breaking become breaking changes. If we have a function

foo :: x -> Maybe y

and we manage to improve it so that we can always return y, eg.

foo :: x -> y

existing callers of our function break. Nullable types a la Kotlin or Typescript seem better. Rich explains this far better than I ever could.

Don't think there's a solution for this in js-land, though!

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a Haskeller, so Rich's talk triggers the @#&! out of me. But what I can certainly agree on is that, yes, there's no satisfactory solution for this in JS-land. ;-)

.then(sent => {
props.setTxnHashCursor(sent)
props.popRoute()
Expand Down
6 changes: 5 additions & 1 deletion src/bridge/lib/txn.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ const signTransaction = async config => {
setStx(Just(stx))
}

const sendSignedTransaction = (web3, stx) => {
const sendSignedTransaction = (web3, stx, confirmationCb) => {
const txn = stx.matchWith({
Just: (tx) => tx.value,
Nothing: () => {
Expand All @@ -155,6 +155,10 @@ const sendSignedTransaction = (web3, stx) => {
.on('receipt', txn => {
resolve(Just(Ok(txn.transactionHash)))
})
.on('confirmation', (confirmationNum, txn) => {
confirmationCb(txn.transactionHash, confirmationNum + 1)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Feels weird to have a callback like this inside a promise, but I think this allows us to implement the feature with minimal code changes to sendSignedTransaction.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah web3's PromiEvents have pretty neat affordances.

This callback currently doesn't seem entirely accurate. The display in Bridge rapidly counts up to 25 (approximately one per second, definitely faster than the block time), then stalls there. In the time it takes it to count up to that, Etherscan has only seen 2 confirmations.

Turns out this is a bug that has been fixed in newer versions of web3. We'll have to try upgrading to latest sometime soon. I'll make an issue for this for the record.

resolve(Just(Ok(txn.transactionHash)))
})
.on('error', err => {
reject(Just(Error(err.message)))
})
Expand Down
1 change: 1 addition & 0 deletions src/bridge/views/AcceptTransfer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ class AcceptTransfer extends React.Component {
walletHdPath={props.walletHdPath}
networkType={props.networkType}
setTxnHashCursor={props.setTxnHashCursor}
setTxnConfirmations={props.setTxnConfirmations}
popRoute={props.popRoute}
pushRoute={props.pushRoute}
// Other
Expand Down
1 change: 1 addition & 0 deletions src/bridge/views/CancelTransfer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ class CancelTransfer extends React.Component {
walletHdPath={props.walletHdPath}
networkType={props.networkType}
setTxnHashCursor={props.setTxnHashCursor}
setTxnConfirmations={props.setTxnConfirmations}
popRoute={props.popRoute}
pushRoute={props.pushRoute}
// Other
Expand Down
1 change: 1 addition & 0 deletions src/bridge/views/CreateGalaxy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,7 @@ class CreateGalaxy extends React.Component {
walletHdPath={props.walletHdPath}
networkType={props.networkType}
setTxnHashCursor={props.setTxnHashCursor}
setTxnConfirmations={props.setTxnConfirmations}
popRoute={props.popRoute}
pushRoute={props.pushRoute}
// Other
Expand Down
1 change: 1 addition & 0 deletions src/bridge/views/IssueChild.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,7 @@ class IssueChild extends React.Component {
walletHdPath={props.walletHdPath}
networkType={props.networkType}
setTxnHashCursor={props.setTxnHashCursor}
setTxnConfirmations={props.setTxnConfirmations}
popRoute={props.popRoute}
pushRoute={props.pushRoute}
// Other
Expand Down
136 changes: 92 additions & 44 deletions src/bridge/views/SentTransaction.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import Maybe from 'folktale/maybe'
import React from 'react'
import { Row, Col, H1, H3, P, Warning, Anchor } from '../components/Base'
import { Button } from '../components/Base'
Expand All @@ -6,55 +7,101 @@ import { ROUTE_NAMES } from '../lib/router'
import { BRIDGE_ERROR, renderTxnError } from '../lib/error'
import { NETWORK_NAMES } from '../lib/network'

const Success = (props) => {

const esvisible =
props.networkType === NETWORK_NAMES.ROPSTEN ||
props.networkType === NETWORK_NAMES.MAINNET

const esdomain =
props.networkType === NETWORK_NAMES.ROPSTEN
? "ropsten.etherscan.io"
: "etherscan.io"

const esmessage =
esvisible === true
? "If you’d like to keep track of it, click the Etherscan link below."
: ''

const esanchor =
esvisible === false
? <div />
: <Anchor
className={'mb-4 mt-1'}
prop-size={'sm'}
target={'_blank'}
href={`https://${esdomain}/tx/${props.hash}`}>
{'View on Etherscan ↗'}
</Anchor>

return (
<Row>
<Col>
<H1>{ 'Your Transaction was Sent' }</H1>
class Success extends React.Component {

constructor(props) {
super(props)

this.state = {
pending: '.',
interval: null
}
}

componentDidMount() {
const nextDot = {'.': '..',
'..': '...',
'...': '.'}

const interval = setInterval(() => {
this.setState(({ pending }) => ({pending: nextDot[pending]}))
}, 1000)
this.setState({interval: interval})
}

componentDidUnmount() {
clearInterval(this.state.interval)
}

render() {

const { props, state } = this
const { networkType, hash, txnConfirmations } = props
const { pending } = state

const esvisible =
networkType === NETWORK_NAMES.ROPSTEN ||
networkType === NETWORK_NAMES.MAINNET

const esdomain =
networkType === NETWORK_NAMES.ROPSTEN
? "ropsten.etherscan.io"
: "etherscan.io"

const esmessage =
esvisible === true
? "If you’d like to keep track of it, click the Etherscan link below."
: ''

const esanchor =
esvisible === false
? <div />
: <Anchor
className={'mb-4 mt-1'}
prop-size={'sm'}
target={'_blank'}
href={`https://${esdomain}/tx/${hash}`}>
{'View on Etherscan ↗'}
</Anchor>

<P>
{
`We sent your transaction to the chain. It can take some time to
const confirmations = Maybe.fromNullable(txnConfirmations[hash]).getOrElse(0)

const requiredConfirmations = 1

const status = confirmations < requiredConfirmations ?
`Pending${pending}` : `Confirmed! (x${confirmations} confirmations)!`

return (
<Row>
<Col>
<H1>{ 'Your Transaction was Sent' }</H1>

<P>
{
`We sent your transaction to the chain. It can take some time to
execute, especially if the network is busy. ${esmessage}`
}
</P>
}
</P>

<H3>{ 'Transaction Hash' }</H3>
<P>
{ props.hash }
</P>
<H3>{ 'Transaction Hash' }</H3>
<P>
{ hash }
</P>

{ esanchor }
<H3>{ 'Transaction Status' }</H3>
<P>
{ status }
{ esanchor }
pkova marked this conversation as resolved.
Show resolved Hide resolved
</P>
<P>
</P>

</Col>
</Row>
)

</Col>
</Row>
)
}
}

const Failure = (props) =>
Expand All @@ -76,7 +123,7 @@ const Failure = (props) =>
</Row>

const SentTransaction = (props) => {
const { web3, txnHashCursor, networkType, popRoute, pushRoute } = props
const { web3, txnHashCursor, networkType, popRoute, pushRoute, txnConfirmations} = props
const { setPointCursor, pointCursor } = props

const promptKeyfile = props.routeData && props.routeData.promptKeyfile
Expand All @@ -97,6 +144,7 @@ const SentTransaction = (props) => {
<Success
hash={ hash.value }
networkType={ networkType }
txnConfirmations={ txnConfirmations }
/>
})

Expand Down
1 change: 1 addition & 0 deletions src/bridge/views/SetKeys.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,7 @@ class SetKeys extends React.Component {
walletHdPath={props.walletHdPath}
networkType={props.networkType}
setTxnHashCursor={props.setTxnHashCursor}
setTxnConfirmations={props.setTxnConfirmations}
popRoute={props.popRoute}
pushRoute={props.pushRoute}
// Tx
Expand Down
1 change: 1 addition & 0 deletions src/bridge/views/SetProxy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,7 @@ class SetProxy extends React.Component {
walletHdPath={props.walletHdPath}
networkType={props.networkType}
setTxnHashCursor={props.setTxnHashCursor}
setTxnConfirmations={props.setTxnConfirmations}
popRoute={props.popRoute}
pushRoute={props.pushRoute}
// Other
Expand Down
1 change: 1 addition & 0 deletions src/bridge/views/Transfer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@ class Transfer extends React.Component {
walletHdPath={props.walletHdPath}
networkType={props.networkType}
setTxnHashCursor={props.setTxnHashCursor}
setTxnConfirmations={props.setTxnConfirmations}
popRoute={props.popRoute}
pushRoute={props.pushRoute}
// Checks
Expand Down