-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Cancel tx JS #4958
Cancel tx JS #4958
Changes from 18 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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,9 @@ | |
# mac stuff | ||
.DS_Store | ||
|
||
# npm stuff | ||
npm-debug.log | ||
|
||
# gdb files | ||
.gdb_history | ||
|
||
|
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 transaction was successfully canceled, `false` if not.', | ||
example: true | ||
} | ||
}, | ||
|
||
registryAddress: { | ||
section: SECTION_NET, | ||
desc: 'The address for the global registry.', | ||
|
@@ -1622,7 +1639,14 @@ export default { | |
example: { | ||
from: '0xb60e8dd61c5d32be8058bb8eb970870f07233155', | ||
to: '0xd46e8dd67c5d32be8058bb8eb970870f07244567', | ||
value: fromDecimal(2441406250) | ||
gas: fromDecimal(30400), | ||
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 just remove the |
||
gasPrice: fromDecimal(10000000000000), | ||
value: fromDecimal(2441406250), | ||
data: '0xd46e8dd67c5d32be8d46e8dd67c5d32be8058bb8eb970870f072445675058bb8eb970870f072445675', | ||
condition: { | ||
block: 354221, | ||
time: new Date() | ||
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. Same with these. |
||
} | ||
} | ||
} | ||
], | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,8 @@ | |
// along with Parity. If not, see <http://www.gnu.org/licenses/>. | ||
|
||
import moment from 'moment'; | ||
import dateDifference from 'date-difference'; | ||
import { FormattedMessage } from 'react-intl'; | ||
import React, { Component, PropTypes } from 'react'; | ||
import { connect } from 'react-redux'; | ||
import { Link } from 'react-router'; | ||
|
@@ -35,6 +37,7 @@ class TxRow extends Component { | |
static propTypes = { | ||
accountAddresses: PropTypes.array.isRequired, | ||
address: PropTypes.string.isRequired, | ||
blockNumber: PropTypes.object, | ||
contractAddresses: PropTypes.array.isRequired, | ||
netVersion: PropTypes.string.isRequired, | ||
tx: PropTypes.object.isRequired, | ||
|
@@ -48,6 +51,13 @@ class TxRow extends Component { | |
historic: true | ||
}; | ||
|
||
state = { | ||
isCancelOpen: false, | ||
isEditOpen: false, | ||
canceled: false, | ||
editing: false | ||
}; | ||
|
||
render () { | ||
const { address, className, historic, netVersion, tx } = this.props; | ||
|
||
|
@@ -137,11 +147,98 @@ 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 { canceled, editing, isCancelOpen, isEditOpen } = this.state; | ||
|
||
if (canceled) { | ||
return ( | ||
<div className={ styles.pending }> | ||
<FormattedMessage | ||
id='ui.txList.txRow.canceled' | ||
defaultMessage='CANCELED' | ||
/> | ||
</div> | ||
); | ||
} | ||
|
||
if (editing) { | ||
return ( | ||
<div className={ styles.pending }> | ||
<FormattedMessage | ||
id='ui.txList.txRow.editing' | ||
defaultMessage='EDITING' | ||
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. It probably would make more sense having the (Same for |
||
/> | ||
</div> | ||
); | ||
} | ||
|
||
if (!isCancelOpen && !isEditOpen) { | ||
return ( | ||
<div className={ styles.pending }> | ||
<span> | ||
<FormattedMessage | ||
id='ui.txList.txRow.time' | ||
defaultMessage='{when}' | ||
values={ { when: this.getCondition() } } | ||
/> | ||
</span> | ||
<div> | ||
<FormattedMessage | ||
id='ui.txList.txRow.scheduled' | ||
defaultMessage='SCHEDULED' | ||
/> | ||
</div> | ||
<a onClick={ this.setEdit }> | ||
<FormattedMessage | ||
id='ui.txList.txRow.edit' | ||
defaultMessage='EDIT' | ||
/> | ||
</a> | ||
<span>{' | '}</span> | ||
<a onClick={ this.setCancel }> | ||
<FormattedMessage | ||
id='ui.txList.txRow.cancel' | ||
defaultMessage='CANCEL' | ||
/> | ||
</a> | ||
</div> | ||
); | ||
} | ||
|
||
return ( | ||
<div className={ styles.pending }> | ||
<div /> | ||
<div> | ||
<FormattedMessage | ||
id='ui.txList.txRow.verify' | ||
defaultMessage='ARE YOU SURE?' | ||
/> | ||
</div> | ||
<a onClick={ (isCancelOpen) ? this.cancelTransaction : this.editTransaction }> | ||
<FormattedMessage | ||
id='ui.txList.txRow.verify.cancelEdit' | ||
defaultMessage='{ which }' | ||
values={ { | ||
which: `${(isCancelOpen) ? 'Cancel' : 'Edit'}` | ||
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. We should get a big red/yellow warning for transaction cancellation since it's not reliable as mentioned in RPC docs. There are two separate cases:
When editing transaction we should also differentiate those two cases. (1) is safe to edit, but (2) should always increase the gas price when edited (it increases the chance of actually replacing the transaction that was already propagated). PM me for more details :) 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. (2) should also zero-out the 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. These should be FormattedMessage as well (Should also be translatable) |
||
} } | ||
/> | ||
</a> | ||
<span>{' | '}</span> | ||
<a onClick={ this.nevermind }> | ||
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 name this something else, e.g. not |
||
<FormattedMessage | ||
id='ui.txList.txRow.verify.nevermind' | ||
defaultMessage='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 +262,61 @@ class TxRow extends Component { | |
|
||
return `/addresses/${address}`; | ||
} | ||
|
||
getCondition = () => { | ||
const { blockNumber, tx } = this.props; | ||
let { time, block } = tx.condition; | ||
|
||
if (time) { | ||
if ((time.getTime() - Date.now()) >= 0) { | ||
return `${dateDifference(new Date(), time, { compact: true })} left`; | ||
} else { | ||
return 'submitting...'; | ||
} | ||
} else if (blockNumber) { | ||
block = blockNumber.minus(block); | ||
return (block.toNumber() < 0) | ||
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 here 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 string returns pulled from |
||
? block.abs().toFormat(0) + ' blocks left' | ||
: 'submitting...'; | ||
} | ||
} | ||
|
||
cancelTransaction = () => { | ||
const { parity } = this.context.api; | ||
const { hash } = this.props.tx; | ||
|
||
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 }); | ||
} | ||
|
||
editTransaction = () => { | ||
const { parity } = this.context.api; | ||
const { hash, gas, gasPrice, to, from, value, input, condition } = this.props.tx; | ||
|
||
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. No need to remove old transaction when editing. If 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 would consider the fact that you can't use the same gas price as a bug. In this case, you can still use the same gas price, which is automated anyways. If the user didn't have the choice for gas price, but rather the GP was automated, than I would just GP++ and purely edit. I should be using promises though... |
||
parity.postTransaction({ | ||
from, | ||
to, | ||
gas, | ||
gasPrice, | ||
value, | ||
condition, | ||
data: input | ||
}); | ||
this.setState({ editing: true }); | ||
} | ||
|
||
setCancel = () => { | ||
this.setState({ isCancelOpen: true }); | ||
} | ||
|
||
setEdit = () => { | ||
this.setState({ isEditOpen: true }); | ||
} | ||
|
||
nevermind = () => { | ||
this.setState({ isCancelOpen: false, isEditOpen: false }); | ||
} | ||
} | ||
|
||
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: 0.75em; | ||
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.25em; | ||
} | ||
} |
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