Filling-in optional fields of TransactionRequest... #1305
Conversation
Conflicts: Cargo.lock
* Deactivate peer if it has no new data * Fixed node table timer registration * Fixed handshake timeout expiration * Extra trace * Fixed session count calculation * Only deactivate incapable peers in ChainHead state * Timer registration is not needed
x64 program files path for installer
firewall rules for windows installer
Re-ahead 8 bytes rather than 3 to ensure large blocks import fine.
Fix read-ahead bug.
* Gas price statistics. Affects eth_gasPrice. Added ethcore_gasPriceStatistics. Closes #1265 * Fix a bug in eth_gasPrice * Fix tests. * Revert minor alteration. * Tests for gas_price_statistics. - Tests; - Additional infrastructure for generating test blocks with transactions.
More meaningful errors when sending transaction
* avoid warning with key * fix intendations * more intendation fix * ok() instead of expect()
|
There were the following issues with your Pull Request
Guidelines are available at https://github.com/ethcore/parity This message was auto-generated by https://gitcop.com |
2 similar comments
|
There were the following issues with your Pull Request
Guidelines are available at https://github.com/ethcore/parity This message was auto-generated by https://gitcop.com |
|
There were the following issues with your Pull Request
Guidelines are available at https://github.com/ethcore/parity This message was auto-generated by https://gitcop.com |
| request.gas = Some(miner.sensible_gas_limit()); | ||
| } | ||
| if let None = request.gas_price { | ||
| request.gas_price = Some(miner.sensible_gas_price()); |
There was a problem hiding this comment.
trace! to help with the reasons that request state mutated?
There was a problem hiding this comment.
It's added in ../impls/mod.rs when sending transaction anyhow, it's not really muted it's just filled in (cause dapp developer left this to us to fill reasonable default)
There was a problem hiding this comment.
well note that value comes from some algorithm and was not set by the user can be helpful, but it's up to you
NikVolf
added
A5-grumble 🔥
tomusdrw
force-pushed
the
signer-fill-optionals
branch
from
1a6add8
to
323de58
Compare
NikVolf
added
A0-pleasereview 🤓
|
|
| } | ||
| } | ||
|
|
||
| fn fill_optional_fields(&self, miner: Arc<M>, mut request: TransactionRequest) -> TransactionRequest { |
There was a problem hiding this comment.
could be worthwhile at some point to make a type-level distinction between requests with fields potentially missing and fully fleshed-out requests, making it impossible to even attempt to use one which isn't complete.
There was a problem hiding this comment.
Yup. In fact it would be best to fill those options in UI, but some of the stuff is not exposed yet. I will create an issue to refactor this part in the (near/post-release) future.
|
Failed test: v1::tests::mocked::eth_signing::should_add_transaction_to_queue |
gavofyork
added
A4-gotissues 💥
Conflicts: rpc/src/v1/impls/eth_signing.rs rpc/src/v1/impls/mod.rs sync/src/chain.rs
tomusdrw
added
A0-pleasereview 🤓
|
|
||
| fn fill_optional_fields(&self, miner: Arc<M>, mut request: TransactionRequest) -> TransactionRequest { | ||
| if let None = request.gas { | ||
| request.gas = Some(miner.sensible_gas_limit()); |
There was a problem hiding this comment.
no - miner.sensible_gas_price is now a fallback and should no longer be used directly. see the implementation of fn default_gas_price() in eth.rs.
There was a problem hiding this comment.
The logic here is the same as in impls/mod.rs#sign_and_dispatch, using signer does not change this behaviour.
Replacing this can go in a separate PR.
gavofyork
added
A4-gotissues 💥
...before sending to Signer UI