Skip to content
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

XVM: default account mapping for Wasm contracts #1056

Merged
merged 5 commits into from
Oct 16, 2023

Conversation

shaunxw
Copy link
Member

@shaunxw shaunxw commented Oct 12, 2023

Pull Request Summary

A quick fix for the first part of #1033. A default EVM address mapping would be added for the Wasm contract if:

  • It's a payable call.
  • There is no mapping yet.

Check list

  • added or updated unit tests
  • updated Astar official documentation
  • added OnRuntimeUpgrade hook for precompile revert code registration
  • updated spec version
  • updated semver

@shaunxw shaunxw added XVM Related to XVM shibuya related to shibuya runtime This PR/Issue is related to the topic “runtime”. labels Oct 12, 2023
@ashutoshvarma
Copy link
Member

ashutoshvarma commented Oct 12, 2023

@shaunxw
Also now that we are using UnifiedAddressMapper trait that can do conversion to and fro for address types, we should remove the AccountMappings trait all together since it's only being used inside XVM and can be replace with UAM trait.

@shaunxw
Copy link
Member Author

shaunxw commented Oct 12, 2023

@shaunxw Also now that we are using UnifiedAddressMapper trait that can do conversion to and fro for address types, we should remove the AccountMappings trait all together since it's only being used inside XVM and can be replace with UAM trait.

XVM is not the only usage, pallet-ethereum-checked is also using it. I agree all the usages can be refactord later to use the UnifiedAddressMapper trait.

@github-actions
Copy link

Code Coverage

Package Line Rate Branch Rate Health
chain-extensions/types/xvm/src 0% 0%
precompiles/assets-erc20/src 76% 0%
pallets/xvm/src 40% 0%
precompiles/sr25519/src 79% 0%
pallets/dynamic-evm-base-fee/src 81% 0%
primitives/src 62% 0%
precompiles/utils/macro/src 0% 0%
pallets/ethereum-checked/src 48% 0%
precompiles/utils/src 55% 0%
precompiles/dapps-staking/src 93% 0%
pallets/contracts-migration/src 0% 0%
chain-extensions/dapps-staking/src 0% 0%
chain-extensions/pallet-assets/src 0% 0%
chain-extensions/xvm/src 0% 0%
precompiles/substrate-ecdsa/src 78% 0%
precompiles/xcm/src 75% 0%
pallets/block-reward/src 85% 0%
chain-extensions/types/assets/src 0% 0%
pallets/unified-accounts/src 80% 0%
precompiles/xvm/src 75% 0%
pallets/xc-asset-config/src 53% 0%
primitives/src/xcm 66% 0%
pallets/dapps-staking/src/pallet 85% 0%
precompiles/utils/src/testing 38% 0%
pallets/dapps-staking/src 81% 0%
pallets/collator-selection/src 69% 0%
chain-extensions/types/dapps-staking/src 0% 0%
Summary 56% (2192 / 3892) 0% (0 / 0)

Minimum allowed line rate is 50%

Copy link
Member

@ashutoshvarma ashutoshvarma left a comment

Choose a reason for hiding this comment

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

Looks good!
We need to update the XVM docs for this as well.


if UA::to_h160(&source).is_none() {
let weight_of_claim = <T as pallet_unified_accounts::Config>::WeightInfo::claim_default_evm_address();
actual_weight.saturating_accrue(weight_of_claim);
Copy link
Member

Choose a reason for hiding this comment

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

I've difficult time understanding, what if actual_weight after adding UA weight is higher than weight_limit?
As I understand, we don't have any check here to make sure such situation doesn't happen.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm aware of this, but didn't want to make the weight handling overcomplicated. It only happens in strict conditions and the weight amount is small. To make it 100% accurate, the weight limit needs to be adjusted as well, and this procedure would repeat everywhere.

Comment on lines -333 to -336

// claim the default mappings for wasm contract
claim_default_accounts(wasm_caller_addr.clone());

Copy link
Member

Choose a reason for hiding this comment

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

Can we add a check for AccountClaimed event for the first payable call to make sure the mappings? thanks

Copy link
Member Author

Choose a reason for hiding this comment

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

It's an integration test, which shouldn't assume how it works under the hood. It's supposed to only check balances to make sure the payable call worked.

Copy link
Contributor

@gitofdeepanshu gitofdeepanshu left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏻

Copy link
Member

@PierreOssun PierreOssun left a comment

Choose a reason for hiding this comment

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

Great PR

@shaunxw shaunxw merged commit 25eec60 into master Oct 16, 2023
@shaunxw shaunxw deleted the feat/xvm-default-account-mapping branch October 16, 2023 01:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
runtime This PR/Issue is related to the topic “runtime”. shibuya related to shibuya XVM Related to XVM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants