Extrinsic Params Refinement #1439

tadeohepperle · 2024-02-22T17:18:47Z

Follow-Up PR to #1432, see discussion between James and me there to get some background information.

This PR introduces a new trait RefineParams that the OtherParams (renamed to Params) of signed extensions must implement:

pub trait RefineParams<T: Config> {
    fn refine(&mut self, _account_nonce: u64, _block_number: u64, _block_hash: T::Hash) {}
}

The 'refinement' of params is executed by an online client before creating an extrinsic and can automatically configure the newest account nonce for CheckNonce and the latest block hash for CheckMortality.

tadeohepperle · 2024-02-22T17:20:52Z

subxt/src/tx/tx_client.rs

+    /// Note: This function will **not** refine the params to use the latest account nonce and the latest block hash/number.
+    /// Instead your call will get default values of nonce = 0 and mortality = Immortal for the signed extensions, if the params
+    /// have not been 'refined' before.
+    pub fn create_partial_signed_offline<Call>(


I am not sure about the name here, maybe create_partial_signed_unrefined? Ideally, we would have different implementations of create_partial_signed for offline and online clients, but without specialization I don't see how that would be possible.

An alternative to specialization with stable rust is the autoref workaround. (James pointed this out a while ago and we ended up using it in substrate for metadata v15)

The default implementation should be implemented on the &T. Whenever users what to override that, they must implement it on T. Then the compiler will add as many references as needed to T to find the method call. Not entirely sure if this is applicable here

Hehe it might be possible but we probably shouldn't go down that route for fear of over-complexifying :D

I like the _offline suffix too FWIW, and also this suffix plays nicely with my suggested comment tweak so that it is consistent that if we call an _offline function we get worse defaults because it runs offline.

Ah I remember reading dtolnays article about autoref specialization on stable rust a while ago, but it seemed like a weird workaround to me.

lexnv · 2024-02-28T11:06:07Z

subxt/src/config/refine_params.rs

+pub trait RefineParams<T: Config> {
+    /// Refine params to an extrinsic. There is usually some notion of 'the param is already set/unset' in types implementing this trait.
+    /// The refinement should most likely not affect cases where a param is in a 'is already set by the user' state.
+    fn refine(&mut self, _account_nonce: u64, _block_number: u64, _block_hash: T::Hash) {}


Are there currently other types that substrate/polkadot extensions take for constructing their state?
I'm thinking more about how we could easily extend this in the future without causing a breaking change

One thing that we could do to maybe minimise the risk of breaking changes a little is to wrap these into a struct like:

pub struct RefinableParamsData<T: Config> { account_nonce: u64, block_number: u64, block_hash: T::Hash } impl <T: Config> RefinableParamsData { pub fn account_nonce(&self) -> u64 { ... } pub fn block_number(&self) -> u64 { ... } pub fn block_hash(&self) -> u64 { ... } }

That way, we can add params without changing the existing interface :)

subxt/src/book/usage/transactions.rs

jsdw · 2024-02-28T13:58:37Z

subxt/src/book/usage/transactions.rs

-//! // here, or can use `create_partial_signed` to fetch the correct nonce.
-//! let partial_tx = client.tx().create_partial_signed_with_nonce(
+//! // Construct the tx but don't sign it. The account nonce here defaults to 0.
+//! // You Can use `create_partial_signed` to fetch the correct nonce.


Suggested change

//! // You Can use `create_partial_signed` to fetch the correct nonce.

//! // You can use `create_partial_signed` to fetch the correct nonce.

jsdw · 2024-02-28T16:07:05Z

subxt/src/config/extrinsic_params.rs

        client: Client,
-        other_params: Self::OtherParams,
+        other_params: Self::Params,


Suggested change

other_params: Self::Params,

params: Self::Params,

subxt/src/config/refine_params.rs

jsdw · 2024-02-28T16:15:30Z

subxt/src/config/mod.rs

@@ -12,6 +12,7 @@ mod default_extrinsic_params;
 mod extrinsic_params;

 pub mod polkadot;
+pub mod refine_params;


I wouldn't export this mod I reckon; there's no real need since the only item it exports is below anyway :)

jsdw · 2024-02-28T16:20:29Z

subxt/src/tx/tx_client.rs

+    /// Fetch the latest block header and account nonce from the backend and use them to refine [`ExtrinsicParams::Params`].
+    pub async fn refine_params(


I wouldn't publically expose this; I think in general that I don't want users to really know about "refining params". If they are offline, they can obtain the relevant data and configure the params exactly as they want, and if they are online then we will be smart and apply some sensible defaults for them is all :)

jsdw · 2024-02-28T16:25:50Z

subxt/src/tx/tx_client.rs

+    /// Note: This function will **not** refine the params to use the latest account nonce and the latest block hash/number.
+    /// Instead your call will get default values of nonce = 0 and mortality = Immortal for the signed extensions, if the params
+    /// have not been 'refined' before.


Suggested change

/// Note: This function will **not** refine the params to use the latest account nonce and the latest block hash/number.

/// Instead your call will get default values of nonce = 0 and mortality = Immortal for the signed extensions, if the params

/// have not been 'refined' before.

/// Note: if not provided, the default account nonce will be set to 0 and the default mortality will be _immortal_.

/// This is because this method runs offline, and so is unable to fetch the data needed for more appropriate values.

Just to make it clear that because offline, we have to use worse defaults, and to avoid mentioning any new concept like "refining params"

jsdw · 2024-02-28T16:26:11Z

subxt/src/tx/tx_client.rs

+    /// Note: This function will **not** refine the params to use the latest account nonce and the latest block hash/number.
+    /// Instead your call will get default values `nonce = 0` and `mortality = Immortal` for the signed extensions, if the
+    /// params have not been manually set or 'refined' before.


Suggested change

/// Note: This function will **not** refine the params to use the latest account nonce and the latest block hash/number.

/// Instead your call will get default values `nonce = 0` and `mortality = Immortal` for the signed extensions, if the

/// params have not been manually set or 'refined' before.

/// Note: if not provided, the default account nonce will be set to 0 and the default mortality will be _immortal_.

/// This is because this method runs offline, and so is unable to fetch the data needed for more appropriate values.

As above

jsdw · 2024-02-28T16:33:07Z

Some small suggestions and such but overall this looks really good, and doesn't feel like it adds too much complexity in the end while giving us the ability to set some great defaults!

It's sortof a shame that we have to ask for the block hash and account nonce etc now every time, but I'm ok with it for now!

(if we think this perf hit is an issue here then we can potentially as an optimisation we could expand the RefinableParams trait with a can_be_refined(&self) -> bool { true } method, and then signed extensions can return false from that when they don't need refining, and if nothing needs refining we can skip obtaining the details at all :))

jsdw

This looks great to me; good job Tadeo!

lexnv

Nice job here!

See paritytech/subxt#1439.

tadeohepperle added 2 commits February 22, 2024 18:11

refinement based approach to params

08d6032

doc and test fixes

a475a70

tadeohepperle commented Feb 22, 2024

View reviewed changes

tadeohepperle mentioned this pull request Feb 22, 2024

BaseParams for SignedExtensions to allow for client-derived defaults in CheckMortality and CheckNonce #1432

Closed

Merge branch 'master' into tadeohepperle/extrinsic-params-refinement

14b796a

tadeohepperle marked this pull request as ready for review February 26, 2024 15:32

tadeohepperle requested a review from a team as a code owner February 26, 2024 15:32

lexnv reviewed Feb 28, 2024

View reviewed changes

subxt/src/book/usage/transactions.rs Outdated Show resolved Hide resolved

jsdw reviewed Feb 28, 2024

View reviewed changes

subxt/src/config/refine_params.rs Show resolved Hide resolved

jsdw reviewed Feb 28, 2024

View reviewed changes

tadeohepperle added 2 commits February 29, 2024 10:56

bundle refine data into struct

9207908

Merge branch 'master' into tadeohepperle/extrinsic-params-refinement

41ec095

jsdw approved these changes Feb 29, 2024

View reviewed changes

Merge branch 'master' into tadeohepperle/extrinsic-params-refinement

5c34dd5

lexnv approved these changes Feb 29, 2024

View reviewed changes

tadeohepperle merged commit 2727f77 into master Mar 1, 2024
13 checks passed

tadeohepperle deleted the tadeohepperle/extrinsic-params-refinement branch March 1, 2024 09:04

jsdw mentioned this pull request Mar 21, 2024

Prep to release 0.35 #1489

Merged

cmichi added a commit to use-ink/cargo-contract that referenced this pull request Mar 28, 2024

Rename OtherParams to Params

cd69a81

See paritytech/subxt#1439.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extrinsic Params Refinement #1439

Extrinsic Params Refinement #1439

tadeohepperle commented Feb 22, 2024

tadeohepperle Feb 22, 2024

lexnv Feb 28, 2024

jsdw Feb 28, 2024

jsdw Feb 28, 2024

tadeohepperle Feb 29, 2024

lexnv Feb 28, 2024

jsdw Feb 28, 2024 •

edited

jsdw Feb 28, 2024

jsdw Feb 28, 2024

jsdw Feb 28, 2024

jsdw Feb 28, 2024

jsdw Feb 28, 2024

jsdw Feb 28, 2024

jsdw commented Feb 28, 2024 •

edited

jsdw left a comment

lexnv left a comment

	//! // You Can use `create_partial_signed` to fetch the correct nonce.
	//! // You can use `create_partial_signed` to fetch the correct nonce.

		/// Fetch the latest block header and account nonce from the backend and use them to refine [`ExtrinsicParams::Params`].
		pub async fn refine_params(

Extrinsic Params Refinement #1439

Extrinsic Params Refinement #1439

Conversation

tadeohepperle commented Feb 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsdw Feb 28, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsdw commented Feb 28, 2024 • edited

jsdw left a comment

Choose a reason for hiding this comment

lexnv left a comment

Choose a reason for hiding this comment

jsdw Feb 28, 2024 •

edited

jsdw commented Feb 28, 2024 •

edited