Impl WeightTrader for tuple

[shaunxw]

This will allow to configure multiple traders for XCM execution fee.

[shawntabrizi]

does this make sense?

We could buy weight with one asset and return weight with another?

[shaunxw]

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.

[shawntabrizi]

yeah that makes sense

[KiChjang]

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.

[shaunxw]

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.

[KiChjang]

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.

[shaunxw]

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.

[shaunxw]

@KiChjang Thanks for helping to add documentations. 👍

[apopiak]

This swallows any errors that might happen along the way. Not sure we want to do that.

[shawntabrizi]

I think the only thing we care about is if all of them fail, which is the final error below. Otherwise, we simply want to check with each trader if it is possible to buy weight, and if not, try the next one.

What do you think might be a better alternative here?

[apopiak]

I think that retry behavior is generally fine, but I think it would be good to at least surface the last error (returned by the last impl of buy_weight).
We should also add some logging (similar to the other XCM tracing I added) and maybe that can be enough.

[apopiak]

I just want to make sure it's possible to debug this at all (which I fear the current version is not).

[shawntabrizi]

is a trace log enough?

[apopiak]

What do you think of:

	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)
	}

[shaunxw]

It's helpful to add a trace log for debugging. Not sure about returning the last error, as it doesn't represent the reason why all traders failed. Probably using a more general error indicating buying weight failed is enough for callers IMHO.

[shaunxw]

Is this PR good to go, or we have to wait for more reviews?

[shawntabrizi]

@shaunxw needs a second approve

[apopiak]

I would like to see some way to get information about the inner errors if this fails so it is more debuggable.

[KiChjang]

bot merge

[ghost]

Trying merge.