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

Impl WeightTrader for tuple #3601

Merged
5 commits merged into from
Aug 18, 2021
Merged

Impl WeightTrader for tuple #3601

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e1a44e9
Impl WeightTrader for tuple.
shaunxw Aug 9, 2021
5407877
fmt
shaunxw Aug 9, 2021
691ce7a
Renaming.
shaunxw Aug 9, 2021
ac73207
add tracing for buy_weight
apopiak Aug 17, 2021
9369bea
Add comment clarifying the default behavior of a WeightTrader tuple
KiChjang Aug 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 50 additions & 0 deletions xcm/xcm-builder/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
// along with Polkadot. If not, see <http://www.gnu.org/licenses/>.

use super::{mock::*, *};
use frame_support::{assert_err, weights::constants::WEIGHT_PER_SECOND};
use xcm::latest::prelude::*;
use xcm_executor::{traits::*, Config, XcmExecutor};

Expand Down Expand Up @@ -372,3 +373,52 @@ fn prepaid_result_of_query_should_get_free_execution() {
let r = XcmExecutor::<TestConfig>::execute_xcm(origin.clone(), message.clone(), weight_limit);
assert_eq!(r, Outcome::Incomplete(10, XcmError::Barrier));
}

fn fungible_multi_asset(location: MultiLocation, amount: u128) -> MultiAsset {
(AssetId::from(location), Fungibility::Fungible(amount)).into()
}

#[test]
fn weight_trader_tuple_should_work() {
pub const PARA_1: MultiLocation = X1(Parachain(1));
pub const PARA_2: MultiLocation = X1(Parachain(2));

parameter_types! {
pub static HereWeightPrice: (AssetId, u128) = (Here.into(), WEIGHT_PER_SECOND.into());
pub static PARA1WeightPrice: (AssetId, u128) = (PARA_1.into(), WEIGHT_PER_SECOND.into());
}

type Traders = (
// trader one
FixedRateOfFungible<HereWeightPrice, ()>,
// trader two
FixedRateOfFungible<PARA1WeightPrice, ()>,
);

let mut traders = Traders::new();
// trader one buys weight
assert_eq!(
traders.buy_weight(5, fungible_multi_asset(Here, 10).into()),
Ok(fungible_multi_asset(Here, 5).into()),
);
// trader one refunds
assert_eq!(traders.refund_weight(2), Some(fungible_multi_asset(Here, 2)));

let mut traders = Traders::new();
// trader one failed; trader two buys weight
assert_eq!(
traders.buy_weight(5, fungible_multi_asset(PARA_1, 10).into()),
Ok(fungible_multi_asset(PARA_1, 5).into()),
);
// trader two refunds
assert_eq!(traders.refund_weight(2), Some(fungible_multi_asset(PARA_1, 2)));

let mut traders = Traders::new();
// all traders fails
assert_err!(
traders.buy_weight(5, fungible_multi_asset(PARA_2, 10).into()),
XcmError::TooExpensive,
);
// and no refund
assert_eq!(traders.refund_weight(2), None);
}
28 changes: 24 additions & 4 deletions xcm/xcm-executor/src/traits/weight.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,11 +76,31 @@ pub trait WeightTrader: Sized {
}
}

impl WeightTrader for () {
#[impl_trait_for_tuples::impl_for_tuples(30)]
impl WeightTrader for Tuple {
fn new() -> Self {
()
for_tuples!( ( #( Tuple::new() ),* ) )
}
fn buy_weight(&mut self, _: Weight, _: Assets) -> Result<Assets, Error> {
Err(Error::Unimplemented)

fn buy_weight(&mut self, weight: Weight, payment: Assets) -> Result<Assets, Error> {
let mut last_error = None;
for_tuples!( #(
match Tuple.buy_weight(weight, payment.clone()) {
Ok(assets) => return Ok(assets),
Err(e) => { last_error = Some(e) }
}
)* );
let last_error = last_error.unwrap_or(Error::TooExpensive);
log::trace!(target: "xcm::buy_weight", "last_error: {:?}", last_error);
Err(last_error)
}

fn refund_weight(&mut self, weight: Weight) -> Option<MultiAsset> {
for_tuples!( #(
if let Some(asset) = Tuple.refund_weight(weight) {
return Some(asset);
}
)* );
Comment on lines +104 to +108
Copy link
Member

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?

Copy link
Contributor Author

@shaunxw shaunxw Aug 9, 2021 •
edited
Loading

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 like FixedRateOfFungible and UsingComponents. 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.

Copy link
Member

Choose a reason for hiding this comment

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

yeah that makes sense

Copy link
Contributor

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 when refund_weight is called, the first WeightTrader in the tuple would refund first, which may not be the correct behaviour.

Copy link
Contributor Author

@shaunxw shaunxw Aug 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

If so, then a WeightTrader tuple may have elements that both accept the non-native token, and when refund_weight is called, the first WeightTrader in the tuple would refund first, which may not be the correct behaviour.

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:

  1. Sufficient USDC. UsdcTrader do the buying and refund. UsdtTrader won't have the chance to try.
  2. Sufficient USDT. UsdtTrader buys and refunds. UsdcTrader tried both first but failed.
  3. Insufficient USDC/USDT. Both trader failed.

Copy link
Contributor

@KiChjang KiChjang Aug 17, 2021 •
edited
Loading

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 a UsdcTrader and USDT has a UsdtTrader. 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 WeightTraders 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 each WeightTrader 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 have Lp1Trader and Lp2Trader in a tuple, and the default behaviour here would then be to always buy and refund with Lp1Trader first, before Lp2Trader 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 2 WeightTraders. However, if that's the case, then I think it would be good to document what the default behaviour is when multiple WeightTraders are put inside of a tuple.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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. 👍

None
}
}