Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify creating and signing extrinsics #490

Merged
merged 13 commits into from
Mar 30, 2022
Merged

Simplify creating and signing extrinsics #490

merged 13 commits into from
Mar 30, 2022

Conversation

jsdw
Copy link
Collaborator

@jsdw jsdw commented Mar 22, 2022

  • Reduce our reliance on substrate traits and move the transaction encoding logic into one place.
  • Simplify the process of defining the additional and extrinsic parameters that are needed for your node, and allow them to be provided to the main entry point methods (sign_and_submit/sign_and_submit_then_watch), adding _default versions of these methods to handle the current case of falling back to default params.
  • In turn, make it possible to provide a tip and mortality for Polkadot/Substrate like transactions at runtime, and add an example (balance_transfer_with_params) to do this.
  • update examples/SCALE to be against Polkadot 0.9.18 to make sure they are working against latest Polkadot.

Comment on lines 282 to 285
signer: &(dyn Signer<T> + Send + Sync),
other_params: X::OtherParams,
) -> Result<Encoded, BasicError> {
// 1. get nonce
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! This does make more sense when following the code logic!

@jsdw jsdw changed the title [WIP] Simplify creating and signing extrinsics Simplify creating and signing extrinsics Mar 24, 2022
/// A default `SignedExtra` configuration, with [`ChargeTransactionPayment`] for tipping.
///
/// Note that this must match the `SignedExtra` type in the target runtime's extrinsic definition.
pub type DefaultExtra<T> = DefaultExtraWithTxPayment<T, ChargeTransactionPayment<T>>;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering how this single DefaultExtra seems to work for both polkadot (in the examples) and substrate (in the tests), because the former just expects a Compact<u128> for the tip section in the extra params, and the latter expects a (Compact<u128>, Option<u32>), which is what this provides via ChargeTransactionPayment. Am I missing something or did we get lucky somehow with that extra 0 byte for the Option<u32> = None being put into the transaction extras in our examples?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ChargeTransactionPayment comes last, so I guess that the node will successfully decode the Compact<u128> and ignore the subsequent data. This data isn't included in AdditionalSigned so won't affect the signature.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd have thought that decoding the call data which comes next would be naffed up in that case! It's a mystery to me at the moment; I'd have to experiment a bit!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Also, the "extra" data is a part of the payload that is signed, so the signature would be different too?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what's going on here then...

Comment on lines +62 to +63
.transfer(dest, 123_456_789_012_345)
.sign_and_submit(&signer, tx_params)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure whether this is possible/desirable, but did you consider adding a builder style method onto SubmittableExtrinsic which allows setting the params, and would use the default if not specified e.g.

let hash = api
        .tx()
        .balances()
        .transfer(dest, 123_456_789_012_345)
        .params(tx_params)
        .sign_and_submit(&signer)

It would avoid the requirement of the _default method equivalents, on the other hand would be less explicit if adding params is the common case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooh interesting! I didn't consider that offhand, no.

One thing I like about having the _default and the param-taking methods is that it's up to the user whether the params actually do implement Default at all or not; the Default constraint is only on those _default methods now, so if for some reason somebody could not or did not want a default set of params for their chain, that'd work just fine.

Copy link
Collaborator Author

@jsdw jsdw Mar 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I say that but actually I think I left a Default clause somewhere I shouldn't have, so this is a good pointer to remove that! done!

subxt/src/client.rs Outdated Show resolved Hide resolved
subxt/src/extrinsic/params.rs Outdated Show resolved Hide resolved
subxt/src/extrinsic/params.rs Show resolved Hide resolved
subxt/src/client.rs Outdated Show resolved Hide resolved
subxt/src/client.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@ascjones ascjones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, a big improvement.

A next step refactoring might be to extract the extrinsic construction in create_signed out of client, which would also allow for "offline" extrinsic construction and also easier unit testing of this critical part of code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants