Skip to content

Girazoki controlled fee specification #658

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

Merged

Conversation

girazoki
Copy link
Contributor

@girazoki girazoki commented Nov 22, 2021

Adds support for specifying the fee as a separate argument to the amount being transferred in xtokens. The fee amount will be used to pay for the buyExecution order in the destination chain. This is useful for cases in which users (or smart contracts) want to have a good control of the amount of tokens in a particular address. The existing extrinsics substract this fee from the amount being transferred, and thus its difficult to know how much tokens a given address has in a specific chain.

For the aforementioned motivation, non-spent fee amounts will get trapped in the destination chain with the new transfer_with_fee and transfer_multiasset_with_fee extrinsics.

For now I made it such that the fee needs to have the same asset id as the token you are transferring. I thought it might be good to test it like this, while we can relax this assumption in the future

Closes #655

@xlc
Copy link
Member

xlc commented Nov 22, 2021

Interesting that you are willing to burn the extra fee rather than give it back to user but I guess I don't have a strong reason to stop you from doing that.

@codecov-commenter
Copy link

codecov-commenter commented Nov 22, 2021

Codecov Report

Merging #658 (41634f8) into master (03fe554) will increase coverage by 0.52%.
The diff coverage is 90.95%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #658      +/-   ##
==========================================
+ Coverage   75.93%   76.46%   +0.52%     
==========================================
  Files          78       78              
  Lines        6549     6772     +223     
==========================================
+ Hits         4973     5178     +205     
- Misses       1576     1594      +18     
Impacted Files Coverage Δ
xtokens/src/mock/para.rs 61.11% <ø> (+8.33%) ⬆️
xtokens/src/lib.rs 73.22% <79.16%> (+3.71%) ⬆️
xtokens/src/tests.rs 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 03fe554...41634f8. Read the comment docs.

Copy link
Member

@shaunxw shaunxw left a comment

Choose a reason for hiding this comment

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

It's worth noting that fee would be burned instead of returned to users in docstring. I'm curious about the reason to burn them.

@girazoki
Copy link
Contributor Author

girazoki commented Nov 23, 2021

The reason for the fee to be burned (or trapped in the destination chain, whatever the destination chain decides to do with non-spent money in the holding register) is that the use case we are trying to cover is that in which:

  • A user (or smart contract) wants to have control over how much tokens the destination account has.
  • The fee that the operation will spend might not be known in advance. With the previous extrinsics, the fee is substracted from the amount and therefore the caller does not have an exact knowledge over how many tokens were trully deposited in the destination.
  • If we return the non-spent fees to the destination, the problem would remain: the caller does not know how many tokens were trully deposited in the account.
  • With the new extrinsic, the caller is accepting the fact that non-spent fees will be burnt, but it knows specifically how many tokens are deposited in the destination account.

We could make it such that we specify, with another argument, the receiver of the non-spent fees. But I think the users will try to adjust quite well the amount of fees they use for the operation so I dont think this would pay-off in the end

@xlc
Copy link
Member

xlc commented Nov 23, 2021

But the code have to handle unexpected token anyway. You cannot prevent others to send some extra token to your account for whatever reason.

@girazoki
Copy link
Contributor Author

girazoki commented Nov 23, 2021

That is actually very right. In that case then maybe we can return the non-used to the deposit account, and what the caller would have is a sense of "minimum balance" that the deposit account should have.

Will modify the PR accordingly. Thanks @xlc!

Copy link
Member

@xlc xlc left a comment

Choose a reason for hiding this comment

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

I think it should error when fee is zero and test case for that.
And some comment indicate if fee is too low, then the asset will be trapped, and some test to check the asset is trapped if that's not too hard.

@girazoki
Copy link
Contributor Author

yeah no problem, I can do that

@girazoki
Copy link
Contributor Author

Is there any reason the tests fail? they work locally for me

@girazoki
Copy link
Contributor Author

I did shortcut also in do_transfer_with_fee for fee==0, before converting it into a fungible, since I think that was the origin of the tests panicking

@shaunxw shaunxw merged commit 5fb024c into open-web3-stack:master Nov 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Be able to specify the fee aside the amount to be transferred in Xtokens
4 participants