This repository has been archived by the owner on Nov 6, 2020. It is now read-only.
import rpc transactions sequentially #10051
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
It looks like @seunlanlege signed our Contributor License Agreement. 👍 Many thanks, Parity Technologies CLA Bot |
seunlanlege
requested review from
sorpaas,
dvdplm,
tomusdrw,
andresilva and
niklasad1
seunlanlege
added
A0-pleasereview 🤓
seunlanlege
commented
Dec 11, 2018
|
There should be some tests that affirm this new behaviour, just how to design them properly 🤔 |
tomusdrw
suggested changes
Dec 13, 2018
There was a problem hiding this comment.
I don't like the fact that now we have logic splitted into multiple places. Also I think ProspectiveDispatcher is uneccessary complication - let's just add PostSign to ProspectiveSigner instead.
I think PostSign should allow custom IntoFuture to be returned.
The result of that future should then be returned as ProspectiveSigner result.
That would simplify the API, the goal would be to have:
let dispatch_result = dispatcher.sign(accounts, filled, password, |transaction| {
dispatcher.dispatch(transaction) // this returns a future
});
// in dispatch_result we have whatever was returned by `dispatcher.dispatch`.
seunlanlege
force-pushed
the
rpc-sequential-tx-import
branch
from
b3e528b
to
c7571f5
Compare
tomusdrw
approved these changes
Dec 14, 2018
| ready: Option<nonce::Ready>, | ||
| post_sign: Option<P>, |
There was a problem hiding this comment.
I know that it's aligned with how currently the state is passed in the ProspectiveSigner, but we might consider refactoring this in a way that the required items are passed in the ProspectiveSignerState enum instead to avoid so many Option< and .expects().
Not required for this PR (or not required at all), unless someone wants to pick it up :)
tomusdrw
approved these changes
Dec 14, 2018
niklasad1
reviewed
Dec 17, 2018
niklasad1
reviewed
Dec 17, 2018
niklasad1
reviewed
Dec 17, 2018
niklasad1
reviewed
Dec 17, 2018
niklasad1
reviewed
Dec 17, 2018
niklasad1
reviewed
Dec 17, 2018
niklasad1
reviewed
Dec 17, 2018
niklasad1
reviewed
Dec 17, 2018
niklasad1
reviewed
Dec 17, 2018
niklasad1
reviewed
Dec 17, 2018
niklasad1
reviewed
Jan 21, 2019
| @@ -491,6 +554,8 @@ impl ProspectiveSigner { | |||
| }, | |||
| prospective: None, | |||
| ready: None, | |||
| post_sign: Some(post_sign), | |||
There was a problem hiding this comment.
It doesn't make sense that post_sign is an Option anymore because poll assumes that it is always is Some otherwise panic and new requires it too!
Then we can get rid of a couple of expect's
There was a problem hiding this comment.
the execute method on PostSign takes self, so inorder to take ownership of post_sign from ProspectiveSigner it needs to be an option. refer to this line
There was a problem hiding this comment.
Ok, thanks for clarifying missed that 👍
niklasad1
previously approved these changes
Jan 21, 2019
niklasad1
added
A6-mustntgrumble 💦
5chdn
dismissed
niklasad1’s stale review
You cannot approve and tag it as "grumble" at the same time. Either tag it as "grumble" or approve it and say "mustntgrumble" ... Dismissing this review.
niklasad1
added
A8-looksgood 🦄
niklasad1
approved these changes
Jan 22, 2019
|
@5chdn we have had many users running into related issues, do you think we could backport this for this or next release? |
|
yes, label at will |
5chdn
added
B1-patch-beta 🕷🕷
B0-patch-stable 🕷
This was referenced Feb 11, 2019
5chdn
pushed a commit
that referenced
this pull request
Feb 12, 2019
* import rpc transactions sequentially * use impl trait in argument position, renamed ProspectiveDispatcher to WithPostSign * grouped imports * integrates PostSign with ProspectiveSigner * fix spaces, removed unnecessary type cast and duplicate polling * clean up code style * Apply suggestions from code review
5chdn
pushed a commit
that referenced
this pull request
Feb 12, 2019
* import rpc transactions sequentially * use impl trait in argument position, renamed ProspectiveDispatcher to WithPostSign * grouped imports * integrates PostSign with ProspectiveSigner * fix spaces, removed unnecessary type cast and duplicate polling * clean up code style * Apply suggestions from code review
5chdn
added a commit
that referenced
this pull request
Feb 13, 2019
* version: bump stable to 2.2.10 * import rpc transactions sequentially (#10051) * import rpc transactions sequentially * use impl trait in argument position, renamed ProspectiveDispatcher to WithPostSign * grouped imports * integrates PostSign with ProspectiveSigner * fix spaces, removed unnecessary type cast and duplicate polling * clean up code style * Apply suggestions from code review * Additional tests for uint deserialization. (#10279) * Don't run the CPP example on CI (#10285) * Don't run the CPP example on CI * Add comment * CI optimizations (#10297) * CI optimizations * fix stripping * new dockerfile * no need n submodule upd * review * moved dockerfile * it becomes large * onchain update depends on s3 * fix dependency * fix cache status * fix cache status * new cache status * fix publish job (#10317) * fix publish job * dashes and colonels * Add Statetest support for Constantinople Fix (#10323) * Update Ethereum tests repo to v6.0.0-beta.3 tag * Add spec for St.Peter's / ConstantinopleFix statetests * fix(add helper for timestamp overflows) (#10330) * fix(add helper timestamp overflows) * fix(simplify code) * fix(make helper private) * fix(docker): fix not receives SIGINT (#10059) * fix(docker): fix not receives SIGINT * fix: update with reviews * update with review * update * update * snap: official image / test (#10168) * official image / test * fix / test * bit more necromancy * fix paths * add source bin/df /test * add source bin/df /test2 * something w paths /test * something w paths /test * add source-type /test * show paths /test * copy plugin /test * plugin -> nil * install rhash * no questions while installing rhash * publish snap only for release * Don't add discovery initiators to the node table (#10305) * Don't add discovery initiators to the node table * Use enums for tracking state of the nodes in discovery * Dont try to ping ourselves * Fix minor nits * Update timeouts when observing an outdated node * Extracted update_bucket_record from update_node * Fixed typo * Fix two final nits from @todr * change docker image based on debian instead of ubuntu due to the chan… (#10336) * change docker image based on debian instead of ubuntu due to the changes of the build container * role back docker build image and docker deploy image to ubuntu:xenial based (#10338) * perform stripping during build (#10208) * perform stripping during build (#10208) * perform stripping during build * var RUSTFLAGS * fix(docker-aarch64) : cross-compile config (#9798) * fix(docker-aarch64) : cross-compile config (#9798) * ci: remove trailing double newline from dockerfile
5chdn
added a commit
that referenced
this pull request
Feb 13, 2019
* version: bump beta to 2.3.3 * import rpc transactions sequentially (#10051) * import rpc transactions sequentially * use impl trait in argument position, renamed ProspectiveDispatcher to WithPostSign * grouped imports * integrates PostSign with ProspectiveSigner * fix spaces, removed unnecessary type cast and duplicate polling * clean up code style * Apply suggestions from code review * Fix Windows build (#10284) * Don't run the CPP example on CI (#10285) * Don't run the CPP example on CI * Add comment * CI optimizations (#10297) * CI optimizations * fix stripping * new dockerfile * no need n submodule upd * review * moved dockerfile * it becomes large * onchain update depends on s3 * fix dependency * fix cache status * fix cache status * new cache status * fix publish job (#10317) * fix publish job * dashes and colonels * Add Statetest support for Constantinople Fix (#10323) * Update Ethereum tests repo to v6.0.0-beta.3 tag * Add spec for St.Peter's / ConstantinopleFix statetests * Properly handle check_epoch_end_signal errors (#10015) * Make check_epoch_end_signal to only use immutable data * Move check_epoch_end_signals out of commit_block * Make check_epoch_end_signals possible to fail * Actually return the error from check_epoch_end_signals * Remove a clone * Fix import error * cargo: fix compilation * fix(add helper for timestamp overflows) (#10330) * fix(add helper timestamp overflows) * fix(simplify code) * fix(make helper private) * Remove CallContract and RegistryInfo re-exports from `ethcore/client` (#10205) * Remove re-export of `CallContract` and `RegistryInfo` from `ethcore/client` * Remove CallContract and RegistryInfo re-exports again This was missed while fixing merge conflicts * fix(docker): fix not receives SIGINT (#10059) * fix(docker): fix not receives SIGINT * fix: update with reviews * update with review * update * update * snap: official image / test (#10168) * official image / test * fix / test * bit more necromancy * fix paths * add source bin/df /test * add source bin/df /test2 * something w paths /test * something w paths /test * add source-type /test * show paths /test * copy plugin /test * plugin -> nil * install rhash * no questions while installing rhash * publish snap only for release * Don't add discovery initiators to the node table (#10305) * Don't add discovery initiators to the node table * Use enums for tracking state of the nodes in discovery * Dont try to ping ourselves * Fix minor nits * Update timeouts when observing an outdated node * Extracted update_bucket_record from update_node * Fixed typo * Fix two final nits from @todr * Extract CallContract and RegistryInfo traits into their own crate (#10178) * Create call-contract crate * Add license * First attempt at using extracted CallContract trait * Remove unneeded `extern crate` calls * Move RegistryInfo trait into call-contract crate * Move service-transaction-checker from ethcore to ethcore-miner * Update Cargo.lock file * Re-export call_contract * Merge CallContract and RegistryInfo imports * Remove commented code * Add documentation to call_contract crate * Add TODO for removal of re-exports * Update call-contract crate description Co-Authored-By: HCastano <HCastano@users.noreply.github.com> * Rename call-contract crate to ethcore-call-contract * Remove CallContract and RegistryInfo re-exports from `ethcore/client` (#10205) * Remove re-export of `CallContract` and `RegistryInfo` from `ethcore/client` * Remove CallContract and RegistryInfo re-exports again This was missed while fixing merge conflicts * fixed: types::transaction::SignedTransaction; (#10229) * fix daemonize dependency * fix build * change docker image based on debian instead of ubuntu due to the chan… (#10336) * change docker image based on debian instead of ubuntu due to the changes of the build container * role back docker build image and docker deploy image to ubuntu:xenial based (#10338) * perform stripping during build (#10208) * perform stripping during build (#10208) * perform stripping during build * var RUSTFLAGS
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously the
ProspectiveSignermarks nonces as used even though they weren't successfully imported into the transaction queuewhich caused issues like: #9040
This PR auguments the nonce reservation system such that a reserved nonce is only marked as used if
the
PostSignaction executes successfully eg transaction is successfully imported into the tx queue