Display local/completed transactions #3630
Conversation
jacogr
added
A3-inprogress ⏳
|
47 years ago
|
| } | ||
|
|
||
| loadBlocks (_blockNumbers) { | ||
| const blockNumbers = Object.keys( |
There was a problem hiding this comment.
Lodash uniq method on requested blocks?
| }); | ||
| } | ||
|
|
||
| loadTransactions (_txhashes) { |
There was a problem hiding this comment.
This should be an @action right ?
There was a problem hiding this comment.
Could be. Technically the only updates happen in the addTransactions function. So @action won't hurt, however the function itself doesn't update the object state by modifying the variables directly. (Split the change out into a different @action - async does not play well with setStrict)
There was a problem hiding this comment.
Mh that's right, didn't think of that. Not sure what it changes.
| fetchLocalTransactions = () => { | ||
| const nextTimeout = () => { | ||
| if (this.doPolling) { | ||
| setTimeout(this.fetchLocalTransactions, 1500); |
There was a problem hiding this comment.
Might be safer to add clearTimeout with the timeout id no?
|
Looks good except the wrong date (when not mined yet) and a few small comments :) |
jacogr
added
A0-pleasereview 🤓
|
Next PR will cover MethodDecoding (pre-confirmation), gas edits & re-submissions. |
| PropTypes.array, | ||
| PropTypes.object | ||
| ]).isRequired, | ||
| isTest: PropTypes.bool.isRequired |
There was a problem hiding this comment.
isTest is required but not provided. I think it should be removed from here.
There was a problem hiding this comment.
Provided via mapStateToProps - however we do have an issue with the new 'undefined' state now. (In multiple places) It is a Required property without doubt.
There was a problem hiding this comment.
Ah yep that's right... So maybe it's not required ? Or it could be defaulted to false in mapStateToProps
There was a problem hiding this comment.
Well, it is required to be provided - if we don't have it, we will get the links wrong. The issue however is that it turned into a tri-state at this point as opposed to the original bool and the isRequired doesn't see it as such, not can it. (We need a different solution for that problem, it is not local to this component - sadly.)
I think that providing a default is hacking around the visibility of the issue.
There was a problem hiding this comment.
Agreed, not in the scope of this issue. Should be null/true/false and the prop type should be a nullable boolean I guess.
There was a problem hiding this comment.
Unfortunately my nullable cleanups are stuck in the I18N PR (and that needs a bit of playing with still on the actual webpack/babel build side of things), will pull it out and make it available so at least we have easier to use functions.
| @@ -44,39 +46,59 @@ class RequestsPage extends Component { | |||
| isTest: PropTypes.bool.isRequired | |||
|
Looks good overall. The Local and Finished transactions seems redundant (we already discussed about it, just wanted to write it down) |
|
Agreed on the Finished. Only benefit now is showing the Rejected transactions, as soon as we have resubmissions in place, it should be dropped and combined with the Local - that way it makes sense, can reject and re-submit again. |
ngotchac
added
A8-looksgood 🦄
Normal transaction list -
Signer list -