Allow setting of minBlock on sending #3921
Conversation
jacogr
commented
Dec 20, 2016
•
jacogr
commented
- Add minBlock to transaction parsing (API)
- Allow minBlock setting for executeContract & Transfer via advanced props
- Closes https://github.com/ethcore/parity/issues/3890
jacogr
added
A3-inprogress ⏳
jacogr
added
A0-pleasereview 🤓
|
Changes Unknown when pulling f4b7450 on jg-minBlock into ** on master**. |
| label={ | ||
| <FormattedMessage | ||
| id='executeContract.advanced.minBlock.label' | ||
| defaultMessage='BlockNumber to send from' /> |
There was a problem hiding this comment.
This indicates that the tx will definitely get mined at the specific block. I'd propose something that is better aligned with the hint above.
|
As a comment: #3594 sounds a lot like it has been designed this way, but I'd prefer the I like the idea of the first stage (e.g. transfer, contract execution) is about what to do, whereas the second stage (signer) is about how to do it. |
| export default class AdvancedStep extends Component { | ||
| static propTypes = { | ||
| gasStore: PropTypes.object.isRequired, | ||
| minBlock: PropTypes.string, |
There was a problem hiding this comment.
[minor grumble] As the block number is an integer, why not use parseInt and make this a PropTypes.number?
| gasStore: PropTypes.object.isRequired, | ||
| minBlock: PropTypes.string, | ||
| minBlockError: PropTypes.string, | ||
| onMinBlockChange: PropTypes.func |
There was a problem hiding this comment.
[minor grumble] Any reason not to make this isRequired?
| @@ -29,29 +30,28 @@ const CHECK_STYLE = { | |||
|
|
|||
| export default class DetailsStep extends Component { | |||
| static propTypes = { | |||
| advancedOptions: PropTypes.bool, | |||
There was a problem hiding this comment.
[tiny grumble] Would prefer something that indicates that this is a boolean, e.g. showAdvancedOptions.
| sending: false, | ||
| step: finalstep, | ||
| txhash, | ||
| busyState: |
There was a problem hiding this comment.
I'd prefer to keep plain strings in the state and use FormattedMessage when rendering.
| }) | ||
| .catch((error) => { | ||
| console.error('postTransaction', error); | ||
| store.dispatch({ type: 'newError', error }); | ||
| }); | ||
| } | ||
|
|
||
| onGasEditClick = () => { | ||
| onAdvancedClick = () => { |
There was a problem hiding this comment.
[tiny grumble] If this was called toggleAdvancedOptions, it would resemble the advancedOptions flag which it toggles. Also, onCheck={ toggleAdvancedOptions } is more readable.
derhuerst
added
A6-mustntgrumble 💦
Signer should indeed be able to tweak it for maximum flexibility, but there's good reason to allow the dapp to request |
|
Changes Unknown when pulling 58d9047 on jg-minBlock into ** on master**. |
