-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Cancel tx JS #4958
Cancel tx JS #4958
Changes from 12 commits
49ed3d8
a9ae42c
eb8fe28
3d31031
eb3e79c
45d1641
f871389
aa47572
35d959e
2235923
65c6390
14dd162
9766508
b57aecd
03c19fd
42dd28a
a9662fa
b7c8c16
89b4343
1aeec64
eb8e56d
131b837
c56ce69
c2ce35c
06c9951
8e17b08
b8a7d62
4d9ca26
684ab6c
c6195d6
85929ea
35aad6c
acf79fe
3c4199b
2c04f54
2d03df7
c9535bf
8d01b28
666f59f
10eda45
ca73bc8
bd05a8c
0f4f40d
3bf3972
fb72ab4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -643,6 +643,23 @@ export default { | |
} | ||
}, | ||
|
||
removeTransaction: { | ||
section: SECTION_ACCOUNTS, | ||
desc: 'Cancel a pending transaction', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it would be good to include a bit more detailed description explaining stuff about no guarantees provided about the transaction being canceled in the network. I tried to describe it in rust docs, maybe you could rephrase this and correct all spelling/grammar errors I made :) |
||
params: [ | ||
{ | ||
type: Data, | ||
desc: 'transactionId, 32-byte hex', | ||
example: '0x1db2c0cf57505d0f4a3d589414f0a0025ca97421d2cd596a9486bc7e2cd2bf8b' | ||
} | ||
], | ||
returns: { | ||
type: Boolean, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually the RPC method returns the transaction if it was removed correctly or |
||
desc: 'returns `true` if the upgrade to the new release was successfully executed, `false` if not.', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Description needs updating |
||
example: true | ||
} | ||
}, | ||
|
||
registryAddress: { | ||
section: SECTION_NET, | ||
desc: 'The address for the global registry.', | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -48,6 +48,11 @@ class TxRow extends Component { | |
historic: true | ||
}; | ||
|
||
state = { | ||
isCancelOpen: false, | ||
canceled: false | ||
}; | ||
|
||
render () { | ||
const { address, className, historic, netVersion, tx } = this.props; | ||
|
||
|
@@ -137,11 +142,46 @@ class TxRow extends Component { | |
return ( | ||
<td className={ styles.timestamp }> | ||
<div>{ blockNumber && block ? moment(block.timestamp).fromNow() : null }</div> | ||
<div>{ blockNumber ? _blockNumber.toFormat() : 'Pending' }</div> | ||
<div>{ blockNumber ? _blockNumber.toFormat() : this.renderCancelToggle() }</div> | ||
</td> | ||
); | ||
} | ||
|
||
renderCancelToggle () { | ||
const { isCancelOpen, canceled } = this.state; | ||
const { parity } = this.context.api; | ||
const { hash } = this.props.tx; | ||
|
||
if (canceled) { | ||
return ( | ||
<div className={ styles.pending }>CANCELED</div> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FormattedMessage? |
||
); | ||
} | ||
|
||
if (!isCancelOpen) { | ||
return ( | ||
<div className={ styles.pending }> | ||
<div>SCHEDULED</div> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FormattedMessage? |
||
<a | ||
className={ styles.cancel } | ||
onClick={ () => this.setState({ isCancelOpen: true }) } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would prefer not using in-line closures, aligning with the codebase. (I actually thought we had a linting rule that warns about these, weird that it wasn't picked up) |
||
> | ||
CANCEL | ||
</a> | ||
</div> | ||
); | ||
} | ||
|
||
return ( | ||
<div className={ styles.pending }> | ||
<div>ARE YOU SURE?</div> | ||
<a onClick={ () => this.cancelTransaction(parity, hash) }>Cancel</a> | ||
<span> | </span> | ||
<a onClick={ () => this.setState({ isCancelOpen: false }) }>Nevermind</a> | ||
</div> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FormattedMessage & inline closures. |
||
); | ||
} | ||
|
||
getIsKnownContract (address) { | ||
const { contractAddresses } = this.props; | ||
|
||
|
@@ -165,6 +205,11 @@ class TxRow extends Component { | |
|
||
return `/addresses/${address}`; | ||
} | ||
|
||
cancelTransaction (parity, hash) { | ||
parity.removeTransaction(hash); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't you wait for the request to actually finish? Also might be good to handle errors. |
||
this.setState({ canceled: true }); | ||
} | ||
} | ||
|
||
function mapStateToProps (initState) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,10 +42,11 @@ | |
} | ||
|
||
&.timestamp { | ||
padding-top: 1.5em; | ||
text-align: right; | ||
max-width: 5em; | ||
padding-top: 2.25em; | ||
text-align: center; | ||
line-height: 1.5em; | ||
opacity: 0.5; | ||
color: grey; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not 100% sure we use this anywhere, would prefer to keep it where it was to keep consistent (We really need to externalise all colours and just pull in CSS variables - no duplication, consistency is maintained) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem is, if opacity is dropped, the "cancel" button will also be 50% opacity LOL. Also, all font is either black or white, 0.5 opacity for both black and white revert to grey anyways. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would keep the original to align with the rest of the UI. (Close enough, but not 100% the same in terms of colors here, we don't use gray elsewhere.) Yes... we need to externalize all color and related vars and just pull them in. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All links and others (sumbitting/ cancel/ edit ...) will be 50% as well. Which is a problem to see/links look strange. I don't mind adding more css, so that the main is 0.5 opacity. But I can't keep the css the way it is. Suggestions are welcome. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahh, ok, the links as well, makes sense. Maybe be more specific then with |
||
} | ||
|
||
&.transaction { | ||
|
@@ -83,4 +84,12 @@ | |
.left { | ||
text-align: left; | ||
} | ||
|
||
.pending { | ||
padding: 0em; | ||
} | ||
|
||
.pending div { | ||
padding-bottom: 1.5em; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -100,7 +100,7 @@ class RequestsPage extends Component { | |
<div className={ styles.noRequestsMsg }> | ||
<FormattedMessage | ||
id='signer.requestsPage.noPending' | ||
defaultMessage='There are no requests requiring your confirmation.' | ||
defaultMessage='There are no transactions or requests requiring your confirmation.' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if I 100% agree with this, although getting heavily into bikeshed territory :) |
||
/> | ||
</div> | ||
</Container> | ||
|
@@ -114,7 +114,7 @@ class RequestsPage extends Component { | |
title={ | ||
<FormattedMessage | ||
id='signer.requestsPage.pendingTitle' | ||
defaultMessage='Pending Requests' | ||
defaultMessage='Pending Transactions' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe we can have more than just pending transaction signing requests available here. @tomusdrw? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah! I am super glad you saw these. I meant to point it out to be disputed. I feel Requests is simply somewhat misleading. We need better terminology, correct? Otherwise, I will drop it. edit - basically, If you say Pending Requests, but you are sending Ether to another person... obviously a debit is not a request. Maybe what would be best is, "Pending Signature Authorization", which would be all inclusive. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They are requests as in "requests for something to be signed". A transaction isn't valid until it's signed. But this interface is also used for more general "sign this random bit of data" as well. I agree that the terminology is a little muddy and clarification would lead to a better UX, but we also have to be careful to remain accurate. |
||
/> | ||
} | ||
> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
0 info it worked if it ends with ok | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please remove. |
||
1 verbose cli [ '/usr/local/Cellar/node/6.1.0/bin/node', | ||
1 verbose cli '/usr/local/bin/npm', | ||
1 verbose cli 'run', | ||
1 verbose cli 'precommit' ] | ||
2 info using npm@3.10.7 | ||
3 info using node@v6.1.0 | ||
4 verbose config Skipping project config: /Users/connor/.npmrc. (matches userconfig) | ||
5 verbose stack Error: ENOENT: no such file or directory, open '/Users/connor/package.json' | ||
5 verbose stack at Error (native) | ||
6 verbose cwd /Users/connor/Desktop/Parity/parity | ||
7 error Darwin 16.4.0 | ||
8 error argv "/usr/local/Cellar/node/6.1.0/bin/node" "/usr/local/bin/npm" "run" "precommit" | ||
9 error node v6.1.0 | ||
10 error npm v3.10.7 | ||
11 error path /Users/connor/package.json | ||
12 error code ENOENT | ||
13 error errno -2 | ||
14 error syscall open | ||
15 error enoent ENOENT: no such file or directory, open '/Users/connor/package.json' | ||
16 error enoent ENOENT: no such file or directory, open '/Users/connor/package.json' | ||
16 error enoent This is most likely not a problem with npm itself | ||
16 error enoent and is related to npm not being able to find a file. | ||
17 verbose exit [ -2, true ] |
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.
transactionHash
would be more adequate