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.

Sign up for GitHub

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

Optional ERC20-XCM gas limit override and tests #2422

Merged
merged 16 commits into from
Aug 28, 2023
Merged

Optional ERC20-XCM gas limit override and tests #2422

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
63bcc50
Gas limit override and tests
fgamundi Aug 6, 2023
d973ff1
EOF line
fgamundi Aug 6, 2023
50fd592
Handle u64
fgamundi Aug 6, 2023
a75ff12
Clean
fgamundi Aug 6, 2023
fccf010
Update pallets/erc20-xcm-bridge/src/lib.rs
fgamundi Aug 8, 2023
37f2fc6
Custom gas limit on weighter
fgamundi Aug 10, 2023
4d51a5a
fmt
fgamundi Aug 10, 2023
8c1ea45
Merge branch 'master' into fg-erc20xcm-manual-gas-limit
fgamundi Aug 14, 2023
6731468
Working with plain u64
fgamundi Aug 14, 2023
9a65a06
Refactor gas limit fn
fgamundi Aug 24, 2023
874524e
Remove tests from old suite
fgamundi Aug 24, 2023
08efaba
Test comment
fgamundi Aug 24, 2023
0f76755
Changed to LE encoded number
fgamundi Aug 25, 2023
303f315
comment
fgamundi Aug 25, 2023
02e0462
Merge branch 'master' into fg-erc20xcm-manual-gas-limit
fgamundi Aug 25, 2023
3d4477b
fix test
fgamundi Aug 25, 2023
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
22 changes: 22 additions & 0 deletions pallets/erc20-xcm-bridge/src/erc20_matcher.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,28 @@ impl<Erc20MultilocationPrefix: Get<MultiLocation>> Erc20Matcher<Erc20Multilocati
_ => Err(()),
}
}
pub(crate) fn matches_gas_limit(multiasset: &MultiAsset) -> Option<u64> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function is useless and increase the call stack for nothing, please remove it and move it's body inside weight_of_erc20_transfer

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you could use if let to write this concisely instead of having a helper fn

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@notlesh tried to follow your suggestion, let me know what you think about the new implementation here

let multilocation = match (&multiasset.fun, &multiasset.id) {
(Fungible(_), Concrete(ref id)) => id,
_ => return None,
};

// let prefix = Erc20MultilocationPrefix::get();
match multilocation.interior().into_iter().next_back() {
Some(Junction::GeneralKey { length, data }) => {
let content = core::str::from_utf8(data).expect("f");
fgamundi marked this conversation as resolved.
Show resolved Hide resolved
if !content.starts_with("gas_limit:") || *length != 32u8 {
return None;
}
// let mut split = content.split(':');
match content.split(':').next_back() {
Some(x) => Some(x.parse().unwrap()),
None => None,
}
}
_ => None,
}
}
}

#[cfg(test)]
Expand Down
28 changes: 21 additions & 7 deletions pallets/erc20-xcm-bridge/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,14 +65,19 @@ pub mod pallet {
pub fn is_erc20_asset(asset: &MultiAsset) -> bool {
Erc20Matcher::<T::Erc20MultilocationPrefix>::is_erc20_asset(asset)
}
pub fn weight_of_erc20_transfer() -> Weight {
T::GasWeightMapping::gas_to_weight(T::Erc20TransferGasLimit::get(), true)
pub fn gas_limit_of_erc20_transfer(asset: &MultiAsset) -> u64 {
fgamundi marked this conversation as resolved.
Show resolved Hide resolved
let gas_limit = Erc20Matcher::<T::Erc20MultilocationPrefix>::matches_gas_limit(asset);
gas_limit.unwrap_or_else(T::Erc20TransferGasLimit::get)
}
pub fn weight_of_erc20_transfer(asset: &MultiAsset) -> Weight {
fgamundi marked this conversation as resolved.
Show resolved Hide resolved
T::GasWeightMapping::gas_to_weight(Self::gas_limit_of_erc20_transfer(asset), true)
}
fn erc20_transfer(
erc20_contract_address: H160,
from: H160,
to: H160,
amount: U256,
gas_limit: u64,
) -> Result<(), Erc20TransferError> {
let mut input = Vec::with_capacity(ERC20_TRANSFER_CALL_DATA_SIZE);
// ERC20.transfer method hash
Expand All @@ -82,15 +87,14 @@ pub mod pallet {
// append amount to be transferred
input.extend_from_slice(H256::from_uint(&amount).as_bytes());

let weight_limit =
T::GasWeightMapping::gas_to_weight(T::Erc20TransferGasLimit::get(), true);
let weight_limit: Weight = T::GasWeightMapping::gas_to_weight(gas_limit, true);

let exec_info = T::EvmRunner::call(
from,
erc20_contract_address,
input,
U256::default(),
T::Erc20TransferGasLimit::get(),
gas_limit,
None,
None,
None,
Expand Down Expand Up @@ -140,6 +144,8 @@ pub mod pallet {
let beneficiary = T::AccountIdConverter::convert_ref(who)
.map_err(|()| MatchError::AccountIdConversionFailed)?;

let gas_limit = Self::gas_limit_of_erc20_transfer(what);

// Get the global context to recover accounts origins.
XcmHoldingErc20sOrigins::with(|erc20s_origins| {
match erc20s_origins.drain(contract_address, amount) {
Expand All @@ -149,7 +155,13 @@ pub mod pallet {
tokens_to_transfer
.into_iter()
.try_for_each(|(from, subamount)| {
Self::erc20_transfer(contract_address, from, beneficiary, subamount)
Self::erc20_transfer(
contract_address,
from,
beneficiary,
subamount,
gas_limit,
)
})
})
.map_err(Into::into),
Expand Down Expand Up @@ -182,10 +194,12 @@ pub mod pallet {
let to = T::AccountIdConverter::convert_ref(to)
.map_err(|()| MatchError::AccountIdConversionFailed)?;

let gas_limit = Self::gas_limit_of_erc20_transfer(asset);

// We perform the evm transfers in a storage transaction to ensure that if it fail
// any contract storage changes are rolled back.
frame_support::storage::with_storage_layer(|| {
Self::erc20_transfer(contract_address, from, to, amount)
Self::erc20_transfer(contract_address, from, to, amount, gas_limit)
})?;

Ok(asset.clone().into())
Expand Down
6 changes: 3 additions & 3 deletions pallets/moonbeam-xcm-benchmarks/src/weights/moonbeam_xcm_benchmarks_fungible.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,21 +32,21 @@ pub struct WeightInfo<T>(PhantomData<T>);
impl<T: frame_system::Config + pallet_erc20_xcm_bridge::Config> WeightInfo<T> {
pub(crate) fn withdraw_asset(asset: &MultiAsset) -> Weight {
if pallet_erc20_xcm_bridge::Pallet::<T>::is_erc20_asset(asset) {
pallet_erc20_xcm_bridge::Pallet::<T>::weight_of_erc20_transfer()
pallet_erc20_xcm_bridge::Pallet::<T>::weight_of_erc20_transfer(asset)
} else {
Weight::from_parts(200_000_000 as u64, ASSET_BURN_MAX_PROOF_SIZE)
}
}
pub(crate) fn transfer_asset(asset: &MultiAsset) -> Weight {
if pallet_erc20_xcm_bridge::Pallet::<T>::is_erc20_asset(asset) {
pallet_erc20_xcm_bridge::Pallet::<T>::weight_of_erc20_transfer()
pallet_erc20_xcm_bridge::Pallet::<T>::weight_of_erc20_transfer(asset)
} else {
Weight::from_parts(200_000_000 as u64, ASSET_TRANSFER_MAX_PROOF_SIZE)
}
}
pub(crate) fn transfer_reserve_asset(asset: &MultiAsset) -> Weight {
if pallet_erc20_xcm_bridge::Pallet::<T>::is_erc20_asset(asset) {
pallet_erc20_xcm_bridge::Pallet::<T>::weight_of_erc20_transfer()
pallet_erc20_xcm_bridge::Pallet::<T>::weight_of_erc20_transfer(asset)
} else {
Weight::from_parts(200_000_000 as u64, ASSET_TRANSFER_MAX_PROOF_SIZE)
}
Expand Down
168 changes: 85 additions & 83 deletions test/suites/dev/test-xcm/test-xcm-erc20-excess-gas.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
sovereignAccountOfSibling,
} from "../../../helpers/xcm.js";
import { parseEther } from "ethers";
import { stringToU8a } from "@polkadot/util";

export const ERC20_TOTAL_SUPPLY = 1_000_000_000n;

Expand Down Expand Up @@ -178,95 +179,96 @@ describeSuite({
},
});

// UNCOMMENT when https://github.com/moonbeam-foundation/moonbeam/pull/2422 is merged

// it({
// id: "T03",
// title: "Incoming ERC20 transfer should succeed if setting a custom gas limit",
// test: async function () {
// const amountTransferred = 1_000_000n;
// const paraSovereign = sovereignAccountOfSibling(context, paraId);
it({
id: "T03",
title: "Incoming ERC20 transfer should succeed if setting a custom gas limit",
test: async function () {
const amountTransferred = 1_000_000n;
const paraSovereign = sovereignAccountOfSibling(context, paraId);

// // Deploy contract
// const { contractAddress, status } = await context.deployContract!("ERC20ExcessGas", {
// args: ["ERC20", "20S", paraSovereign, ERC20_TOTAL_SUPPLY],
// });
// expect(status).eq("success");
// Deploy contract
const { contractAddress, status } = await context.deployContract!("ERC20ExcessGas", {
args: ["ERC20", "20S", paraSovereign, ERC20_TOTAL_SUPPLY],
});
expect(status).eq("success");

// // Get pallet indices
// const metadata = await polkadotJs.rpc.state.getMetadata();
// const balancesPalletIndex = metadata.asLatest.pallets
// .find(({ name }) => name.toString() == "Balances")!
// .index.toNumber();
// const erc20XcmPalletIndex = metadata.asLatest.pallets
// .find(({ name }) => name.toString() == "Erc20XcmBridge")!
// .index.toNumber();
// Get pallet indices
const metadata = await polkadotJs.rpc.state.getMetadata();
const balancesPalletIndex = metadata.asLatest.pallets
.find(({ name }) => name.toString() == "Balances")!
.index.toNumber();
const erc20XcmPalletIndex = metadata.asLatest.pallets
.find(({ name }) => name.toString() == "Erc20XcmBridge")!
.index.toNumber();

// // Send some native tokens to the sovereign account of paraId (to pay fees)
// await polkadotJs.tx.balances.transfer(paraSovereign, parseEther("1")).signAndSend(alith);
// await context.createBlock();
// Send some native tokens to the sovereign account of paraId (to pay fees)
await polkadotJs.tx.balances.transfer(paraSovereign, parseEther("1")).signAndSend(alith);
await context.createBlock();

// // Create the incoming xcm message
// const config: XcmFragmentConfig = {
// assets: [
// {
// multilocation: {
// parents: 0,
// interior: {
// X1: { PalletInstance: Number(balancesPalletIndex) },
// },
// },
// fungible: 100_000_000_000_000_000n,
// },
// {
// multilocation: {
// parents: 0,
// interior: {
// X3: [
// {
// PalletInstance: erc20XcmPalletIndex,
// },
// {
// AccountKey20: {
// network: "Any",
// key: contractAddress,
// },
// },
// {
// GeneralIndex: 300_000n,
// },
// ],
// },
// },
// fungible: amountTransferred,
// },
// ],
// beneficiary: CHARLETH_ADDRESS,
// };
// Create the incoming xcm message
const config: XcmFragmentConfig = {
assets: [
{
multilocation: {
parents: 0,
interior: {
X1: { PalletInstance: Number(balancesPalletIndex) },
},
},
fungible: 100_000_000_000_000_000n,
},
{
multilocation: {
parents: 0,
interior: {
X3: [
{
PalletInstance: erc20XcmPalletIndex,
},
{
AccountKey20: {
network: "Any",
key: contractAddress,
},
},
{
GeneralKey: {
data: "gas_limit:0000000000000000300000",
length: 32,
},
},
],
},
},
fungible: amountTransferred,
},
],
beneficiary: CHARLETH_ADDRESS,
};

// const xcmMessage = new XcmFragment(config)
// .withdraw_asset()
// .clear_origin()
// .buy_execution()
// .deposit_asset_v3(2n)
// .as_v3();
const xcmMessage = new XcmFragment(config)
.withdraw_asset()
.clear_origin()
.buy_execution()
.deposit_asset_v3(2n)
.as_v3();

// // Mock the reception of the xcm message
// await injectHrmpMessageAndSeal(context, paraId, {
// type: "XcmVersionedXcm",
// payload: xcmMessage,
// });
// Mock the reception of the xcm message
await injectHrmpMessageAndSeal(context, paraId, {
type: "XcmVersionedXcm",
payload: xcmMessage,
});

// // Charleth should have received the tokens
// expect(
// await context.readContract!({
// contractName: "ERC20ExcessGas",
// contractAddress: contractAddress as `0x${string}`,
// functionName: "balanceOf",
// args: [CHARLETH_ADDRESS],
// })
// ).equals(amountTransferred);
// },
// });
// Charleth should have received the tokens
expect(
await context.readContract!({
contractName: "ERC20ExcessGas",
contractAddress: contractAddress as `0x${string}`,
functionName: "balanceOf",
args: [CHARLETH_ADDRESS],
})
).equals(amountTransferred);
},
});
},
});
Loading