-
Notifications
You must be signed in to change notification settings - Fork 107
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
Add command to update registerTokenFee and sendTokenFee #991
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #991 +/- ##
==========================================
- Coverage 83.36% 82.69% -0.67%
==========================================
Files 51 51
Lines 2044 2069 +25
Branches 71 72 +1
==========================================
+ Hits 1704 1711 +7
- Misses 325 343 +18
Partials 15 15
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
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. Needs a few naming related changes, and then we should be ready to go.
|
||
function testSetTokenFees() public { | ||
GatewayMock(address(gateway)).setTokenTransferFeesPublic( | ||
abi.encode(Gateway.SetTokenTransferFeesParams({register: 1, send: 1})) |
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.
The default fees are already (1, 1), so this test could pass even if the command failed somehow.
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.
Actually the default are 1(ether) not 1(wei), anyway check fee before and after set and make sure change as expected.
0a2cb16
register: u128, | ||
send: 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.
Lets validate these to make sure they are non-zero, and if not, return an InvalidTokenFees
error.
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.
parachain/pallets/control/src/lib.rs
Outdated
/// Sends a message to the Gateway contract to update fees | ||
/// | ||
/// Privileged. Can only be called by root. | ||
/// | ||
/// Fee required: No | ||
/// | ||
/// - `origin`: Must be root | ||
/// - `location`: Location used to resolve the agent | ||
/// - `recipient`: Recipient of funds | ||
/// - `amount`: Amount to transfer |
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.
Documentation needs to be updated.
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.
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.
A few minor comments. Looks great! 👍🏻
Co-authored-by: Clara van Staden <[email protected]>
Co-authored-by: Clara van Staden <[email protected]>
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. We should add an integration test for this.
Resolves: sno-738
Requires: Snowfork/polkadot-sdk#15