-
Notifications
You must be signed in to change notification settings - Fork 254
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
Partial fee estimates for SubmittableExtrinsic #910
Conversation
Bumps [h2](https://github.com/hyperium/h2) from 0.3.16 to 0.3.17. - [Release notes](https://github.com/hyperium/h2/releases) - [Changelog](https://github.com/hyperium/h2/blob/master/CHANGELOG.md) - [Commits](hyperium/h2@v0.3.16...v0.3.17) --- updated-dependencies: - dependency-name: h2 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
.rpc() | ||
.state_call(function, call_parameters, Some(block_hash)) | ||
.request( | ||
"state_call", |
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 do wonder if we could use the exposed state_call
under our rpc.rs methods.
Generally, we can avoid making plain rpc requests and rely on the exposed functionality of the rpc.rs.
This has the following advantages:
- no need to remember and hardcode the function name.
Placing static str in the code base can be limited to the rpc wrappers, which results in better code maintenance. We also get better error handling. Making a call torpc.state_cccall()
is a compile error while calling"state_cccall"
will be detected at runtime - Parameters are defined in the function signature, which hints at the values we'd need to pass in. On the other hand the
rpc_params
can take in any parameters (potentially incomplete).
Let me know what you think of it 🙏
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.
Yeah; let's just make the result of this call_raw
be Res: Decode
:)
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.
@lexnv Good idea. Should I change something in the code now, or is that a thing for another PR?
@jsdw I would like to make your suggested change. But the function name should stay call_raw
? So the raw then only refers to the call_parameters: Option<&'a [u8]>
and not to the return type anymore, right?
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.
Yeah, keep the call_raw
; this is because in the future we'll be adding a much more ergonomic call
method which will be the preferred way to use this interface :)
And please feel free to make the change in this PR so that we can keep the code simple given the definition of the state_call
RPC method!
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.
LGTM, nice work
// Method I: TransactionPaymentApi_query_info | ||
let partial_fee_1 = signed_extrinsic.partial_fee_estimate().await.unwrap(); | ||
|
||
// Method II: TransactionPaymentApi_query_fee_details + calculations |
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 wonder if it's sufficient to just rely on that the result from Method I: TransactionPaymentApi_query_info
could be decoded as this should already be tested by substrate I reckon.
Then remove this extra code 👇, but this is a nice way to check the result so I have mixed feelings here your call
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'm happy either way too; I don't think we need the test but it's interesting in documenting how the things work so I'm easy :)
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.
If the logic changes in the future the test will break, then we can still change/remove it I think :)
// destructuring RuntimeDispatchInfo, see type information <https://paritytech.github.io/substrate/master/pallet_transaction_payment_rpc_runtime_api/struct.RuntimeDispatchInfo.html> | ||
// data layout: {weight_ref_time: Compact<u64>, weight_proof_size: Compact<u64>, class: u8, partial_fee: u128} |
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.
Nice; more compact code with nice comment to explain :)
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.
Looks good to me; nice job!
Partially fixes #527.
There are two rpc endpoints that can be used in principle to obtain an estimate of the partial_fee. That is the paid fee without tips. I added a function
partial_fee_estimate()
toSubmittableExtrinsic
, that uses Method I, as it is simpler.Method I: TransactionPaymentApi_query_info
The struct
RuntimeDispatchInfo
returned by this endpoint contains a the partial_fee in question:Here the
partial_fee
is already the result we are looking fMethod II: TransactionPaymentApi_query_fee_details
Make a state call to "TransactionPaymentApi_query_fee_details
Referring to this this stackexchange answer by jsdw:
partial_fee = base_fee + len_fee + (adjusted_weight_fee/estimated_weight*actual_weight)
We assume here, that estimated_weight == actual_weight.
So this should hold and give the same result as Method I:
Tests:
I added an integration test that verifies both methods work and return the same result.