This repository has been archived by the owner on Nov 15, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Impl WeightTrader for tuple #3601
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
does this make sense?
We could buy weight with one asset and return weight with another?
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.
From my understanding, a sensible
WeightTrader
impl would be that if weights are not bought here, then it won't refund. This is the case for all current traders likeFixedRateOfFungible
andUsingComponents
. The tuple impl based on them would refund the same asset with which weights were bought.This is covered in the added unit tests
weight_trader_tuple_should_work
.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 that makes sense
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.
That seems to be the case if we assume that weight can only be bought by the native token of a chain. I'm trying to understand if this is still valid for assets that are non-native, e.g. USDC on Statemint/Statemine.
Would it be possible to buy weights using non-native tokens? If so, then a
WeightTrader
tuple may have elements that both accept the non-native token, and whenrefund_weight
is called, the firstWeightTrader
in the tuple would refund first, which may not be the correct behaviour.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 tuple impl tries buy/refund weights in the same order. If I understand your example correctly, in this case, the first trader buys weight, so it's fine that it refunds.
For instance, we have a tuple with two non-native token traders
(UsdcTrader, UsdtTrader)
, if the assets are:UsdcTrader
do the buying and refund.UsdtTrader
won't have the chance to try.UsdtTrader
buys and refunds.UsdcTrader
tried both first but failed.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.
That seems to carry an assumption that each token has a corresponding
WeightTrader
associated with it, e.g. USDC has aUsdcTrader
and USDT has aUsdtTrader
. Is this relationship guaranteed to always be 1:1? If so, then this logic looks to be sufficient enough.What I'm more concerned about is when you have multiple
WeightTrader
s that references the same token to trade. I'm imagining that some DeFi protocols may be permissive enough to allow for multiple tokens to be used to pay for fees, and eachWeightTrader
in their use case corresponds to a liquidity pool. Let's say both traders reference USDC as a token that they accept for paying for the weights. You'll then haveLp1Trader
andLp2Trader
in a tuple, and the default behaviour here would then be to always buy and refund withLp1Trader
first, beforeLp2Trader
gets queried, which may result in an incorrect behaviour.Though practically speaking, such a use case is complex enough to justify for a custom implementation of the
WeightTrader
trait on a self-defined type, and I wouldn't expect the developers of such a DeFi protocol to simply use a tuple to wrap the 2WeightTrader
s. However, if that's the case, then I think it would be good to document what the default behaviour is when multipleWeightTrader
s are put inside of a tuple.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 tuple impl doesn't assume 1:1 between tokens and traders. It's similar to
SendXcm
tuple that making no assumption of who sends what kind of messages, but simply try one by one until the msg is successfully sent. I think it's reasonable to make only one of the traders buy/refund weights, as the required weights shouldn't be bought/refunded twice, just like an XCM message should be sent twice. And yeah it's helpful to document this behavior.I agree that for more complex requirements, ppl should really impl a trader themselves instead of expecting this tuple impl to help. Actually we're planning to impl a DEX trader in Karura, like in your example, and it will be used with other backup traders together.
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.
@KiChjang Thanks for helping to add documentations. 👍