Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Add payment_queryFeeDetails RPC #7692

Merged
41 commits merged into from
Jan 14, 2021
Merged

Add payment_queryFeeDetails RPC #7692

41 commits merged into from
Jan 14, 2021

Conversation

liuchengxu
Copy link
Contributor

@liuchengxu liuchengxu commented Dec 8, 2020 •
edited
Loading

This PR adds a new RPC payment_queryFeeDetails which returns the details of the final fee, there are no changes to the existing payment RPC.

Close #7691

kianenigma
kianenigma reviewed Dec 16, 2020
View reviewed changes
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

Looks overall good, would be good if someone can look closer into the serde details as well.

@shawntabrizi shawntabrizi added the C1-low PR touches the given topic and has a low impact on builders. label Jan 1, 2021
gui1117
gui1117 reviewed Jan 4, 2021
View reviewed changes
frame/transaction-payment/rpc/src/lib.rs Outdated Show resolved Hide resolved
primitives/rpc/src/number.rs Outdated Show resolved Hide resolved
gui1117
gui1117 approved these changes Jan 4, 2021
View reviewed changes
Copy link
Contributor

@gui1117 gui1117 left a 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

frame/transaction-payment/src/types.rs Outdated Show resolved Hide resolved
@liuchengxu @gui1117
Update frame/transaction-payment/src/types.rs
b02d85d 
Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
@gui1117 gui1117 added A0-please_review Pull request needs code review. and removed A0-please_review Pull request needs code review. labels Jan 4, 2021
@gui1117
Copy link
Contributor

gui1117 commented Jan 4, 2021

at some point I wonder if those two RPC shouldn't be merged, or just the new RPC have all information so that we don't break RPCs. But I'm ok like this.

@liuchengxu liuchengxu mentioned this pull request Jan 4, 2021
Merged
@kianenigma kianenigma added the D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited label Jan 4, 2021
@kianenigma
Copy link
Contributor

^^ the refactor that also affects the runtime code is minimal, but still, I think this is the correct next step. Correct me otherwise.

bkchr
bkchr reviewed Jan 13, 2021
View reviewed changes
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

We also don't bump the version of the runtime api. As it will only return error when calling the RPC and the runtime api doesn't exist, I'm fine with it.

frame/transaction-payment/src/types.rs Outdated Show resolved Hide resolved
frame/transaction-payment/src/types.rs Outdated Show resolved Hide resolved
frame/transaction-payment/src/types.rs Outdated Show resolved Hide resolved
frame/transaction-payment/src/types.rs Outdated Show resolved Hide resolved
frame/transaction-payment/rpc/src/lib.rs Outdated Show resolved Hide resolved
frame/transaction-payment/rpc/src/lib.rs Outdated Show resolved Hide resolved
@gui1117 gui1117 added D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Jan 14, 2021
@bkchr
Copy link
Member

bkchr commented Jan 14, 2021

bot merge

@ghost
Copy link

ghost commented Jan 14, 2021

Trying merge.

@ghost ghost merged commit f2367f7 into paritytech:master Jan 14, 2021
@liuchengxu liuchengxu mentioned this pull request Jan 14, 2021
Closed
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@xlc xlc xlc left review comments

@bkchr bkchr bkchr approved these changes

@gui1117 gui1117 gui1117 approved these changes

@kianenigma kianenigma Awaiting requested review from kianenigma

Assignees
No one assigned
Labels
B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow querying the fee details from transaction payment
7 participants
@liuchengxu @gui1117 @kianenigma @bkchr @xlc @shawntabrizi @paritytech-ci