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.

Sign up for GitHub

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

Jump to bottom

update transaction fee xpallet #561

Merged
merged 20 commits into from
Jun 23, 2021
Merged

update transaction fee xpallet #561

merged 20 commits into from
Jun 23, 2021

Conversation

rain-9811
Copy link

No description provided.

@liuchengxu liuchengxu requested review from hacpy and 0xrjman June 15, 2021 14:37
liuchengxu
liuchengxu suggested changes Jun 16, 2021
View reviewed changes
Copy link
Contributor

@liuchengxu liuchengxu left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Please delete all the useless lines.

For the call having extra fee, have you tested locally that xfee_queryDetails works exactly like before? It would be even better if you can add a test accordingly.

@liuchengxu liuchengxu requested a review from alannotnerd June 17, 2021 01:58
liuchengxu
liuchengxu suggested changes Jun 17, 2021
View reviewed changes
Copy link
Contributor

@liuchengxu liuchengxu left a comment

Choose a reason for hiding this comment

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

The previous copy-pasted code should be removed, e.g., compute_fee_raw().

@rain-9811
Copy link
Author

The transaction fee result is the same like before , the updated pallet result is
image
while the partial pallet result is
image

@alannotnerd
Copy link
Contributor

Close #442

0xrjman
0xrjman reviewed Jun 18, 2021
View reviewed changes
Copy link

@0xrjman 0xrjman left a comment

Choose a reason for hiding this comment

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

The rest is LGTM

xpallets/transaction-fee/src/lib.rs Outdated Show resolved Hide resolved
xpallets/transaction-fee/rpc/src/lib.rs Outdated Show resolved Hide resolved
xpallets/transaction-fee/rpc/src/lib.rs Outdated Show resolved Hide resolved
xpallets/transaction-fee/src/lib.rs Show resolved Hide resolved
xpallets/transaction-fee/src/lib.rs Show resolved Hide resolved
@0xrjman 0xrjman self-requested a review June 18, 2021 03:42
0xrjman
0xrjman reviewed Jun 18, 2021
View reviewed changes
Copy link

@0xrjman 0xrjman left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Extra Comment

alannotnerd
alannotnerd reviewed Jun 18, 2021
View reviewed changes
xpallets/transaction-fee/rpc/runtime-api/src/lib.rs Outdated
Comment on lines 1 to 3
// // Copyright 2019-2020 ChainX Project Authors. Licensed under GPL-3.0.

//! Runtime API definition for transaction fee module.
// //! Runtime API definition for transaction fee module.
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove duplicated slash

alannotnerd
alannotnerd reviewed Jun 18, 2021
View reviewed changes
runtime/dev/src/lib.rs Outdated
Comment on lines 1358 to 1362
xpallet_transaction_fee::FeeDetails {
inclusion_fee: base_clone.inclusion_fee,
tip: base.tip,
extra_fee: extra_fee.into(),
final_fee: total + extra_fee,
Copy link
Contributor

Choose a reason for hiding this comment

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

Format code

@liuchengxu liuchengxu merged commit 92454ad into develop Jun 23, 2021
@liuchengxu liuchengxu deleted the update-transaction-fee-xpallet branch June 23, 2021 07:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xrjman 0xrjman 0xrjman left review comments

@alannotnerd alannotnerd alannotnerd left review comments

@liuchengxu liuchengxu liuchengxu requested changes

@hacpy hacpy Awaiting requested review from hacpy

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rain-9811 @alannotnerd @liuchengxu @0xrjman