Feat/xcm precompile update #996
Conversation
My bad, I have updated it now. |
precompiles/xcm/src/lib.rs
Outdated
|
|
||
| let asset_id = Runtime::address_to_asset_id(currency_address.into()) | ||
| .ok_or(revert("Failed to resolve fee asset id from address"))?; | ||
| let dest_weight_limit = if weight.is_max() { |
There was a problem hiding this comment.
The interface docstring should cover when Unlimited would be used.
Personally, I think zero is a more conventional value. For max weight, it has indicated unlimited by using itself.
There was a problem hiding this comment.
I have added the info about Unlimited weight in interface.
Personally, I think zero is a more conventional value. For max weight, it has indicated unlimited by using itself.
Not sure what you mean by this.
There was a problem hiding this comment.
Personally, I think zero is a more conventional value. For max weight, it has indicated unlimited by using itself.
Not sure what you mean by this.
I mean weight max vs zero to indicate Unlimited. is_zero should be used instead of is_max.
There was a problem hiding this comment.
for this case is_max make more sense imo. Can we go ahead with this?
There was a problem hiding this comment.
This is part of the public API so let's be cautious.
Zero value is conventional to indicate unlimited/unstrict. Using a uint64 max value is counter-intuition and clumsy to use. In addition, when a max weight is passed, there is actually no need to convert it to Unlimited as it's unlimited itself.
There was a problem hiding this comment.
Agree with Shaun about zero as the sentinel value.
It's harder to mess up, even if in the future we change some things related to weight type (probably won't, but still). Zero will always be a zero.
There was a problem hiding this comment.
Make sense, I was not able to understand what exactly you meant but now I get it.
So you are saying that Weight = 0 , should result into WeightLimit::Unlimited and not u64::MAX value.
So what you are suggesting is this
let dest_weight_limit = if weight.is_zero() {
WeightLimit::Unlimited
} else {
WeightLimit::Limited(weight.get_weight())
};
I got the idea of using u64::MAX for Unlimited from MB but this is a better approach, I agree.
There was a problem hiding this comment.
Now that Moonbeam's precompile-utils is included in frontier and we can remove the vendor code (precompiles-utils) after the next uplift.
gitofdeepanshu
added
shiden
There was a problem hiding this comment.
@ashutoshvarma please create an issue to follow-up on this: #996 (review)
| let amount_of_tokens = input | ||
| .read::<U256>()? | ||
| .try_into() | ||
| .map_err(|_| revert("error converting amount_of_tokens, maybe value too large"))?; |
There was a problem hiding this comment.
Keeping the style consistent in the same file, that's added in its entirety in your PR, is not nit-picky.
When someone reviews a big PR such as this one, you want to make it as easy as possible.
Doing the same thing in multiple different ways doesn't help with that, nor with maintenance in the future.
precompiles/xcm/src/lib.rs
Outdated
|
|
||
| let asset_id = Runtime::address_to_asset_id(currency_address.into()) | ||
| .ok_or(revert("Failed to resolve fee asset id from address"))?; | ||
| let dest_weight_limit = if weight.is_max() { |
There was a problem hiding this comment.
Agree with Shaun about zero as the sentinel value.
It's harder to mess up, even if in the future we change some things related to weight type (probably won't, but still). Zero will always be a zero.
Minimum allowed line rate is |
Pull Request Summary
This PR adds multiple new features to xcm precompile.
Changes
send_xcm()which allows to send any xcm-message to the destination chain.Multilocationto precompileThis addition helps in getting rid of function overloading for different Account types (
AccountId32andAccountKey20) supported by xcm.3. Support for updated weight using
so that user can specify the
proof_sizeof weight.4. Addition of
XtokensInterfaceCheck list