This repository has been archived by the owner on Nov 6, 2020. It is now read-only.
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.
import rpc transactions sequentially #10051
import rpc transactions sequentially #10051
Changes from all commits
d9c85b2
8f51117
69466f7
c7571f5
24b0e7b
726dba8
001a42a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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 theProspectiveSignerState
enum instead to avoid so manyOption<
and.expects()
.Not required for this PR (or not required at all), unless someone wants to pick it up :)
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.
Just a quick idea (maybe dumb idea :) , maybe this trait could be more generic. PostSign is specific name, but as I understand it is a definition for a future post action on a signed transaction, if we make the signed transaction (
WithToken<SignedTransaction>
) an inner type (input) we got something that is totally generic (I wonder if it maybe already exists in the future api something like a trait for implementing a combination offuture::ok
andand_then
).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.
the futures crate has no such api
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.
It doesn't make sense that
post_sign
is an Option anymore becausepoll
assumes that it is always isSome
otherwise panic andnew
requires it too!Then we can get rid of a couple of
expect's
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.
the
execute
method onPostSign
takesself
, so inorder to take ownership ofpost_sign
fromProspectiveSigner
it needs to be an option. refer to this linehttps://github.com/paritytech/parity-ethereum/blob/001a42a98a557edc7c602265a580dd552c871a1d/rpc/src/v1/helpers/dispatch.rs#L592-L599
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.
Ok, thanks for clarifying missed that 👍