Add transaction status indicator to SentTransaction #99

pkova · 2019-04-08T20:44:54Z

Fixes #93

pkova · 2019-04-08T20:47:19Z

src/bridge/lib/txn.js

@@ -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)


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.

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.

Fang- · 2019-04-08T23:09:29Z

src/bridge/Bridge.js

-                txnHashCursor={ txnHashCursor } />
+                txnHashCursor={ txnHashCursor }
+                setTxnConfirmations={ this.setTxnConfirmations }
+                txnConfirmations={ txnConfirmations } />


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

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.

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.

jtobin · 2019-04-08T23:11:32Z

src/bridge/components/StatelessTransaction.js

@@ -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)


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

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"? (^: )

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> ) } }

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

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!

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

src/bridge/views/SentTransaction.js

Fang-

This is great! Thank you so much @pkova, really happy to have this! (:

I left some comments. Architecture/style discussion aside, this is almost good to be merged in.

Fang- · 2019-04-09T13:55:27Z

src/bridge/lib/txn.js

@@ -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)


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.

src/bridge/views/SentTransaction.js

src/bridge/Bridge.js

pkova · 2019-04-16T17:36:48Z

Animations are a go.

Fang-

Last little rendering nit! Once that's fixed, I'll merge this, and make a new Bridge release to put it live. (:

src/bridge/views/SentTransaction.js

React complains about having divs inside of p elements otherwise

Fang- · 2019-04-16T19:24:54Z

Looks clean! Thanks a bunch @pkova, this is good!

Add transaction status indicator to SentTransaction

58ac642

pkova commented Apr 8, 2019

View reviewed changes

alexmatz requested a review from Fang- April 8, 2019 22:12

Fang- reviewed Apr 8, 2019

View reviewed changes

jtobin reviewed Apr 8, 2019

View reviewed changes

Fang- reviewed Apr 8, 2019

View reviewed changes

src/bridge/views/SentTransaction.js Outdated Show resolved Hide resolved

Fang- requested changes Apr 9, 2019

View reviewed changes

Fang- mentioned this pull request Apr 9, 2019

Use latest web3 #101

Closed

pkova added 2 commits April 16, 2019 20:28

Animate pending dots

31aa349

Dont put divs inside p

765deb1

Fang- requested changes Apr 16, 2019

View reviewed changes

src/bridge/views/SentTransaction.js Outdated Show resolved Hide resolved

Give status an esanchor their own paragraphs

bf869de

Fang- approved these changes Apr 16, 2019

View reviewed changes

pkova added 3 commits April 16, 2019 21:35

Fix bug in interval clearing on component unmount

e2a43bb

Remove dangling p element

d67f269

Return nothing instead of empty div when esvisible == false

8de9bc3

React complains about having divs inside of p elements otherwise

Fang- merged commit 58b8e51 into urbit:master Apr 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add transaction status indicator to SentTransaction #99

Add transaction status indicator to SentTransaction #99

pkova commented Apr 8, 2019

pkova Apr 8, 2019

Fang- Apr 9, 2019

Fang- Apr 8, 2019

jtobin Apr 8, 2019

c-johnson Apr 9, 2019 •

edited

Loading

jtobin Apr 8, 2019

Fang- Apr 8, 2019

jtobin Apr 8, 2019

Fang- Apr 8, 2019

pkova Apr 9, 2019

jtobin Apr 10, 2019

Fang- left a comment

Fang- Apr 9, 2019

pkova commented Apr 16, 2019

Fang- left a comment

Fang- commented Apr 16, 2019

Add transaction status indicator to SentTransaction #99

Add transaction status indicator to SentTransaction #99

Conversation

pkova commented Apr 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c-johnson Apr 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fang- left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkova commented Apr 16, 2019

Fang- left a comment

Choose a reason for hiding this comment

Fang- commented Apr 16, 2019

c-johnson Apr 9, 2019 •

edited

Loading