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

Snowbridge: deposit extra fee to beneficiary on Asset Hub #4175

Merged
merged 16 commits into from
Apr 26, 2024

Conversation

yrong
Copy link
Contributor

@yrong yrong commented Apr 17, 2024

Just the upper-stream for Snowfork#137 and more context there.

@yrong yrong marked this pull request as ready for review April 17, 2024 12:43
@paritytech-review-bot paritytech-review-bot bot requested a review from a team April 17, 2024 12:44
Comment on lines 276 to 279
// Deposit both asset and fees to beneficiary so the fees will not get
// trapped. Another benefit is when fees left more than ED on AssetHub could be
// used to create the beneficiary account in case it does not exist.
DepositAsset { assets: Wild(All), beneficiary },
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe whole XCM fails if the beneficiary account does not exist and the leftover fees do not satisfy ED... :(

Maybe add an SetErrorHandler and fall back to deposit just the assets if the wildcard one fails.

I recommend building a test or more to validate both happy and corner cases.

Copy link
Contributor Author

@yrong yrong Apr 17, 2024

Choose a reason for hiding this comment

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

Thanks for review it.

fall back to deposit just the assets if the wildcard one fails.

Currently WETH or any foreign asset from Ethereum is created as non_sufficient, so the fall back won't help in this case.

the leftover fees do not satisfy ED

As mentioned in Snowfork#137 (comment), we'll ensure this won't happen.

building a test or more to validate both happy and corner cases.

Sure, I'll do that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would avoid using DepositAsset(Wild(All)) because it can be quite expensive, e.g on the AssetHub here: https://github.com/paritytech/polkadot-sdk/blob/master/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/weights/xcm/mod.rs#L33-L51. It's better to use either Wild(AllOfCounted(2)) to deposit both the asset and fees, or to use Definite([asset, fees]).

We encountered a very similar issue raised by the security audit team here: #3157, and there was also a recommendation to use SetAppendix to avoid potential trapping.

Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend building a test or more to validate both happy and corner cases.

👆 💯

Copy link
Contributor Author

Choose a reason for hiding this comment

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

More tests added in
0a218c7

@bkontur I'm not sure SetAppendix will help in this case, the DepositAsset is the last instruction of the xcm and if any of the preceding instructions fails, we do want the xcm fail as a whole.

Copy link
Contributor

Choose a reason for hiding this comment

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

SetAppendix

if you put something in the holding register (e.g. WithdrawAsset...) and another instruction fails, then the assets are trapped. But if you use SetAppendix(DepositAssets and place it before the failing instruction, then assets won't be trapped in the case of error, but instead Deposited to some account, so no asset trapping.

But it depends case-by-case, I'm not saying that you need to use it, I am just saying that there is this possibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the reminder. I'll let our team to double check though I do think DepositAsset should be the last instruction here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bkontur Confirmed that we don't need the SetAppendix. We'll just leave the asset trapped when error happens.

@paritytech-review-bot paritytech-review-bot bot requested a review from a team April 18, 2024 12:58
Comment on lines 670 to 687
#[test]
fn send_token_from_ethereum_to_non_existent_account_on_asset_hub_with_sufficient_fee_but_do_not_satisfy_ed(
) {
// On AH the xcm fee is 33_873_024 and the ED is 3_300_000
send_token_from_ethereum_to_asset_hub_with_fee([1; 32], 36_000_000);

AssetHubRococo::execute_with(|| {
type RuntimeEvent = <AssetHubRococo as Chain>::RuntimeEvent;

// Check that the token was received and issued as a foreign asset on AssetHub
assert_expected_events!(
AssetHubRococo,
vec![
RuntimeEvent::MessageQueue(pallet_message_queue::Event::Processed { success:false, .. }) => {},
]
);
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

so it looks like it does fail, losing the ETH as well - was better off before when extra fees would just get trapped - at least then ETH always went through

I think you should do a SetErrorHandler(DepositAssets(JustEth)) so that if DepositAssets(AllCounted(2)) fails, you fall back to depositing just the ETH.

Later, when we have #4190 in XCMv5, we can simplify and just trap "unhandled" assets with no worries as they can be easily claimed.

Copy link
Contributor Author

@yrong yrong Apr 18, 2024

Choose a reason for hiding this comment

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

Yeah, we do need the custom-asset-claimer feature. Currently it's quite a challenge to claim the trapped WETH if error happens.

@acatangiu In this commit I do add SetErrorHandler(DepositAssets(JustEth)), but it does not help as I mentioned because WETH is created as non_sufficient asset.

As you can see from the log the test send_token_from_ethereum_to_non_existent_account_on_asset_hub_with_sufficient_fee_but_do_not_satisfy_ed still fails and the xcm execution cost increased(as more instructions added).

So I'm not sure if it provides benefit here.

cargo test -p bridge-hub-rococo-integration-tests --lib tests::snowbridge::send_token_from_ethereum_to_non_existent_account_on_asset_hub_with_sufficient_fee_but_do_not_satisfy_ed  -- --nocapture

2024-04-18T14:51:52.625030Z TRACE xcm::process_instruction: === ReceiveTeleportedAsset(Assets([Asset { id: AssetId(Location { parents: 1, interior: Here }), fun: Fungible(48000000) }]))
2024-04-18T14:51:52.625037Z TRACE xcm::ensure_can_subsume_assets: worst_case_holding_len: 1, holding_limit: 64
2024-04-18T14:51:52.625075Z TRACE xcm::contains: ConcreteAssetFromSystem asset: Asset { id: AssetId(Location { parents: 1, interior: Here }), fun: Fungible(48000000) }, origin: Location { parents: 1, interior: X1([Parachain(1013)]) }
2024-04-18T14:51:52.625083Z TRACE xcm::fungible_adapter: can_check_in origin: Location { parents: 1, interior: X1([Parachain(1013)]) }, what: Asset { id: AssetId(Location { parents: 1, interior: Here }), fun: Fungible(48000000) }
2024-04-18T14:51:52.625102Z TRACE xcm::fungible_adapter: check_in origin: Location { parents: 1, interior: X1([Parachain(1013)]) }, what: Asset { id: AssetId(Location { parents: 1, interior: Here }), fun: Fungible(48000000) }
2024-04-18T14:51:52.625120Z TRACE xcm::fungibles_adapter: check_in origin: Location { parents: 1, interior: X1([Parachain(1013)]) }, what: Asset { id: AssetId(Location { parents: 1, interior: Here }), fun: Fungible(48000000) }
2024-04-18T14:51:52.625128Z TRACE xcm::fungibles_adapter: check_in origin: Location { parents: 1, interior: X1([Parachain(1013)]) }, what: Asset { id: AssetId(Location { parents: 1, interior: Here }), fun: Fungible(48000000) }
2024-04-18T14:51:52.625148Z TRACE xcm::fungibles_adapter: check_in origin: Location { parents: 1, interior: X1([Parachain(1013)]) }, what: Asset { id: AssetId(Location { parents: 1, interior: Here }), fun: Fungible(48000000) }
2024-04-18T14:51:52.625155Z TRACE xcm::nonfungibles_adapter: check_in origin: Location { parents: 1, interior: X1([Parachain(1013)]) }, what: Asset { id: AssetId(Location { parents: 1, interior: Here }), fun: Fungible(48000000) }, context: XcmContext { origin: Some(Location { parents: 1, interior: X1([Parachain(1013)]) }), message_id: [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0], topic: None }
2024-04-18T14:51:52.625251Z TRACE xcm::process_instruction: === BuyExecution { fees: Asset { id: AssetId(Location { parents: 1, interior: Here }), fun: Fungible(48000000) }, weight_limit: Limited(Weight { ref_time: 511962000, proof_size: 13757 }) }
2024-04-18T14:51:52.625291Z TRACE xcm::weight: UsingComponents::buy_weight weight: Weight { ref_time: 511962000, proof_size: 13757 }, payment: AssetsInHolding { fungible: {AssetId(Location { parents: 1, interior: Here }): 48000000}, non_fungible: {} }, context: XcmContext { origin: Some(Location { parents: 1, interior: X1([Parachain(1013)]) }), message_id: [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0], topic: None }
2024-04-18T14:51:52.625322Z TRACE xcm::process_instruction: === DescendOrigin(X1([PalletInstance(80)]))
2024-04-18T14:51:52.625333Z TRACE xcm::process_instruction: === UniversalOrigin(GlobalConsensus(Ethereum { chain_id: 11155111 }))
2024-04-18T14:51:52.625379Z TRACE xcm::process_instruction: === ReserveAssetDeposited(Assets([Asset { id: AssetId(Location { parents: 2, interior: X2([GlobalConsensus(Ethereum { chain_id: 11155111 }), AccountKey20 { network: None, key: [135, 209, 247, 253, 254, 231, 246, 81, 250, 188, 139, 252, 182, 224, 134, 194, 120, 183, 122, 125] }]) }), fun: Fungible(1000000) }]))
2024-04-18T14:51:52.625388Z TRACE xcm::ensure_can_subsume_assets: worst_case_holding_len: 2, holding_limit: 64
2024-04-18T14:51:52.625413Z TRACE xcm::contains: IsTrustedBridgedReserveLocationForConcreteAsset asset: Asset { id: AssetId(Location { parents: 2, interior: X2([GlobalConsensus(Ethereum { chain_id: 11155111 }), AccountKey20 { network: None, key: [135, 209, 247, 253, 254, 231, 246, 81, 250, 188, 139, 252, 182, 224, 134, 194, 120, 183, 122, 125] }]) }), fun: Fungible(1000000) }, origin: Location { parents: 2, interior: X1([GlobalConsensus(Ethereum { chain_id: 11155111 })]) }, universal_source: X2([GlobalConsensus(Rococo), Parachain(1000)])
2024-04-18T14:51:52.625425Z TRACE xcm::contains: Case asset: Asset { id: AssetId(Location { parents: 2, interior: X2([GlobalConsensus(Ethereum { chain_id: 11155111 }), AccountKey20 { network: None, key: [135, 209, 247, 253, 254, 231, 246, 81, 250, 188, 139, 252, 182, 224, 134, 194, 120, 183, 122, 125] }]) }), fun: Fungible(1000000) }, origin: Location { parents: 2, interior: X1([GlobalConsensus(Ethereum { chain_id: 11155111 })]) }
2024-04-18T14:51:52.625437Z TRACE xcm::contains: IsForeignConcreteAsset asset: Asset { id: AssetId(Location { parents: 2, interior: X2([GlobalConsensus(Ethereum { chain_id: 11155111 }), AccountKey20 { network: None, key: [135, 209, 247, 253, 254, 231, 246, 81, 250, 188, 139, 252, 182, 224, 134, 194, 120, 183, 122, 125] }]) }), fun: Fungible(1000000) }, origin: Location { parents: 2, interior: X1([GlobalConsensus(Ethereum { chain_id: 11155111 })]) }
2024-04-18T14:51:52.625458Z TRACE xcm::process_instruction: === ClearOrigin
2024-04-18T14:51:52.625464Z TRACE xcm::process_instruction: === SetErrorHandler(Xcm([DepositAsset { assets: Definite(Assets([Asset { id: AssetId(Location { parents: 2, interior: X2([GlobalConsensus(Ethereum { chain_id: 11155111 }), AccountKey20 { network: None, key: [135, 209, 247, 253, 254, 231, 246, 81, 250, 188, 139, 252, 182, 224, 134, 194, 120, 183, 122, 125] }]) }), fun: Fungible(1000000) }])), beneficiary: Location { parents: 0, interior: X1([AccountId32 { network: None, id: [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1] }]) } }]))
2024-04-18T14:51:52.625474Z TRACE xcm::weight: WeightInfoBounds message: Xcm([DepositAsset { assets: Definite(Assets([Asset { id: AssetId(Location { parents: 2, interior: X2([GlobalConsensus(Ethereum { chain_id: 11155111 }), AccountKey20 { network: None, key: [135, 209, 247, 253, 254, 231, 246, 81, 250, 188, 139, 252, 182, 224, 134, 194, 120, 183, 122, 125] }]) }), fun: Fungible(1000000) }])), beneficiary: Location { parents: 0, interior: X1([AccountId32 { network: None, id: [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1] }]) } }])
2024-04-18T14:51:52.625485Z TRACE xcm::process_instruction: === DepositAsset { assets: Wild(AllCounted(2)), beneficiary: Location { parents: 0, interior: X1([AccountId32 { network: None, id: [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1] }]) } }
2024-04-18T14:51:52.625523Z TRACE xcm::fungible_adapter: deposit_asset what: Asset { id: AssetId(Location { parents: 1, interior: Here }), fun: Fungible(2143334) }, who: Location { parents: 0, interior: X1([AccountId32 { network: None, id: [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1] }]) }
2024-04-18T14:51:52.625586Z TRACE xcm::execute: !!! ERROR: FailedToTransactAsset("Account cannot exist with the funds that would be given")
2024-04-18T14:51:52.625596Z TRACE xcm::execute: result: Err(ExecutorError { index: 7, xcm_error: FailedToTransactAsset("Account cannot exist with the funds that would be given"), weight: Weight { ref_time: 2241000, proof_size: 0 } })
2024-04-18T14:51:52.625602Z TRACE xcm::process: origin: None, total_surplus/refunded: Weight { ref_time: 2241000, proof_size: 0 }/Weight { ref_time: 0, proof_size: 0 }, error_handler_weight: Weight { ref_time: 0, proof_size: 0 }
2024-04-18T14:51:52.625608Z TRACE xcm::process_instruction: === DepositAsset { assets: Definite(Assets([Asset { id: AssetId(Location { parents: 2, interior: X2([GlobalConsensus(Ethereum { chain_id: 11155111 }), AccountKey20 { network: None, key: [135, 209, 247, 253, 254, 231, 246, 81, 250, 188, 139, 252, 182, 224, 134, 194, 120, 183, 122, 125] }]) }), fun: Fungible(1000000) }])), beneficiary: Location { parents: 0, interior: X1([AccountId32 { network: None, id: [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1] }]) } }
2024-04-18T14:51:52.625659Z TRACE xcm::fungible_adapter: deposit_asset what: Asset { id: AssetId(Location { parents: 2, interior: X2([GlobalConsensus(Ethereum { chain_id: 11155111 }), AccountKey20 { network: None, key: [135, 209, 247, 253, 254, 231, 246, 81, 250, 188, 139, 252, 182, 224, 134, 194, 120, 183, 122, 125] }]) }), fun: Fungible(1000000) }, who: Location { parents: 0, interior: X1([AccountId32 { network: None, id: [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1] }]) }
2024-04-18T14:51:52.625670Z TRACE xcm::fungibles_adapter: deposit_asset what: Asset { id: AssetId(Location { parents: 2, interior: X2([GlobalConsensus(Ethereum { chain_id: 11155111 }), AccountKey20 { network: None, key: [135, 209, 247, 253, 254, 231, 246, 81, 250, 188, 139, 252, 182, 224, 134, 194, 120, 183, 122, 125] }]) }), fun: Fungible(1000000) }, who: Location { parents: 0, interior: X1([AccountId32 { network: None, id: [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1] }]) }
2024-04-18T14:51:52.625680Z TRACE xcm::fungibles_adapter: deposit_asset what: Asset { id: AssetId(Location { parents: 2, interior: X2([GlobalConsensus(Ethereum { chain_id: 11155111 }), AccountKey20 { network: None, key: [135, 209, 247, 253, 254, 231, 246, 81, 250, 188, 139, 252, 182, 224, 134, 194, 120, 183, 122, 125] }]) }), fun: Fungible(1000000) }, who: Location { parents: 0, interior: X1([AccountId32 { network: None, id: [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1] }]) }
2024-04-18T14:51:52.625817Z TRACE xcm::execute: !!! ERROR: FailedToTransactAsset("Account cannot be created")
2024-04-18T14:51:52.625824Z TRACE xcm::execute: result: Err(ExecutorError { index: 0, xcm_error: FailedToTransactAsset("Account cannot be created"), weight: Weight { ref_time: 0, proof_size: 0 } })
2024-04-18T14:51:52.625829Z TRACE xcm::refund_surplus: total_surplus: Weight { ref_time: 2241000, proof_size: 0 }, total_refunded: Weight { ref_time: 0, proof_size: 0 }, current_surplus: Weight { ref_time: 2241000, proof_size: 0 }
2024-04-18T14:51:52.625834Z TRACE xcm::weight: UsingComponents::refund_weight weight: Weight { ref_time: 2241000, proof_size: 0 }, context: XcmContext { origin: None, message_id: [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0], topic: None }, available weight: Weight { ref_time: 511962000, proof_size: 13757 }, available amount: 45856666
2024-04-18T14:51:52.625892Z TRACE xcm::weight: UsingComponents::refund_weight amount to refund: 6976
2024-04-18T14:51:52.625902Z TRACE xcm::refund_surplus: total_refunded: Weight { ref_time: 2241000, proof_size: 0 }
2024-04-18T14:51:52.626082Z TRACE xcm::post_process: Trapping assets in holding register: AssetsInHolding { fungible: {AssetId(Location { parents: 1, interior: Here }): 2150310, AssetId(Location { parents: 2, interior: X2([GlobalConsensus(Ethereum { chain_id: 11155111 }), AccountKey20 { network: None, key: [135, 209, 247, 253, 254, 231, 246, 81, 250, 188, 139, 252, 182, 224, 134, 194, 120, 183, 122, 125] }]) }): 1000000}, non_fungible: {} }, context: XcmContext { origin: None, message_id: [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0], topic: None } (original_origin: Location { parents: 1, interior: X1([Parachain(1013)]) })
2024-04-18T14:51:52.626174Z TRACE xcm::post_process: Execution errored at 0: FailedToTransactAsset("Account cannot be created") (original_origin: Location { parents: 1, interior: X1([Parachain(1013)]) })
2024-04-18T14:51:52.626182Z TRACE xcm::process-message: XCM message execution incomplete, used weight: Weight(ref_time: 509721000, proof_size: 13757), error: FailedToTransactAsset("Account cannot be created")
...

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, that's right, it doesn't help since account doesn't exist and WETH can't count towards ED..

I guess we'll just have to trap for now

@paritytech-review-bot paritytech-review-bot bot requested a review from a team April 18, 2024 14:57
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5981019

@yrong
Copy link
Contributor Author

yrong commented Apr 22, 2024

Do I need to provide anything more for this PR get merged?

@acatangiu
Copy link
Contributor

Do I need to provide anything more for this PR get merged?

Looks good to go to me. You just need a prdoc (example: https://github.com/paritytech/polkadot-sdk/blob/master/prdoc/pr_4118.prdoc)

@yrong
Copy link
Contributor Author

yrong commented Apr 23, 2024

@bkontur Any other concern or could you help to approve the PR?

@yrong yrong requested a review from bkontur April 23, 2024 01:50
yrong and others added 2 commits April 23, 2024 15:10
…idge-hub-rococo/src/tests/snowbridge.rs

Co-authored-by: Clara van Staden <claravanstaden64@gmail.com>
…idge-hub-rococo/src/tests/snowbridge.rs

Co-authored-by: Clara van Staden <claravanstaden64@gmail.com>
@acatangiu acatangiu added the T15-bridges This PR/Issue is related to bridges. label Apr 25, 2024
@acatangiu acatangiu changed the title Deposit extra fee to beneficiary on asset hub Snowbridge: deposit extra fee to beneficiary on Asset Hub Apr 25, 2024
prdoc/pr_4175.prdoc Outdated Show resolved Hide resolved
prdoc/pr_4175.prdoc Outdated Show resolved Hide resolved
// Deposit both asset and fees to beneficiary so the fees will not get
// trapped. Another benefit is when fees left more than ED on AssetHub could be
// used to create the beneficiary account in case it does not exist.
DepositAsset { assets: Wild(AllCounted(2)), beneficiary },
Copy link
Contributor

@bkontur bkontur Apr 25, 2024

Choose a reason for hiding this comment

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

You could maybe add description before every instruction for let mut instructions = vec![ :)

One question: could you point me to the place or explain where asset_hub_fee and destination.fee is coming from? Is it withdrawn or burned somewhere on BH? Is it the user's money, or is it money from some sovereign account on BH? Or where does this money come from?

I think it can be specific (not sure if compiles):

Suggested change
DepositAsset { assets: Wild(AllCounted(2)), beneficiary },
DepositAsset {
assets: Definite(vec![asset, asset_hub_fee_asset].into())),
beneficiary
},

or maybe use total_fee_asset(dest_para_fee should be 0 in this branch) instead of asset_hub_fee_asset?

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't you need to fix also DepositReserveAsset { branch?

// Deposit asset to beneficiary.
DepositAsset { assets: Definite(asset.into()), beneficiary },

Copy link
Contributor

Choose a reason for hiding this comment

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

One question: could you point me to the place or explain where asset_hub_fee and destination.fee is coming from? Is it withdrawn or burned somewhere on BH? Is it the user's money, or is it money from some sovereign account on BH? Or where does this money come from?

My concern here is whether we should deposit unused fees directly to the beneficiary instead of depositing them into a sovereign account in the AH. If this is a valid scenario, then the process should resemble something like this:

DepositAsset { assets: Definite(vec![asset].into())), beneficiary},
DepositAsset { assets: Definite(vec![asset_hub_fee_asset].into())), beneficiary: LocationOfSomeSovereignAccountOnAssetHub},

Copy link
Contributor Author

Choose a reason for hiding this comment

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

where asset_hub_fee and destination.fee is coming from?

asset_hub_fee from https://github.com/Snowfork/snowbridge/blob/3f90b1619d0bdbec8f50b63185b3dcc632321019/contracts/src/storage/AssetsStorage.sol#L15 which is a configurable storage on Ethereum, we charge it from the user and make sure that it can cover the execution cost on AH.

destination.fee is always zero for AH.

whether we should deposit unused fees directly to the beneficiary

I think so, it's from end user and we do want to send the unused back to the user.

Don't you need to fix also DepositReserveAsset { branch?

Nope, that branch is for non-system parachain which may not accept AssetHub-DOT as fee asset, we're gonna to make some fix there later and this PR focus on AH only.

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 so, it's from end user and we do want to send the unused back to the user.

ok, cool, thank you :)

Don't you need to fix also DepositReserveAsset { branch?

Nope, that branch is for non-system parachain which may not accept AssetHub-DOT as fee asset, we're gonna to make some fix there later and this PR focus on AH only.

Ok, got it. I saw that you use Location::parent() here for fees.assetId, so that would be probably changed and/or Destination.fee: u128, let's see later.

So, I think using exact assets for DepositAsset, as suggested above, is not a bad idea (or maybe some exact counter of unique assetIds :))

Co-authored-by: Adrian Catangiu <adrian@parity.io>
Co-authored-by: Adrian Catangiu <adrian@parity.io>
@yrong
Copy link
Contributor Author

yrong commented Apr 26, 2024

@acatangiu Could you help to merge the PR? Good to know that we're gonna to make WETH as sufficient on AH. Though IMO this PR still make sense for any other non-sufficient asset from Ethereum.

@acatangiu
Copy link
Contributor

@yrong please merge master for the CI fix

@serban300 serban300 added this pull request to the merge queue Apr 26, 2024
@yrong
Copy link
Contributor Author

yrong commented Apr 26, 2024

There is one CI task gitlab-check-runtime-migration-westend failed https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6071070 but seems not related to this PR.

Merged via the queue into paritytech:master with commit d893cde Apr 26, 2024
138 of 139 checks passed
Morganamilo pushed a commit that referenced this pull request May 2, 2024
Just the upper-stream for
Snowfork#137 and more context
there.

---------

Co-authored-by: Clara van Staden <claravanstaden64@gmail.com>
Co-authored-by: Adrian Catangiu <adrian@parity.io>
TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this pull request Aug 2, 2024
…#4175)

Just the upper-stream for
Snowfork#137 and more context
there.

---------

Co-authored-by: Clara van Staden <claravanstaden64@gmail.com>
Co-authored-by: Adrian Catangiu <adrian@parity.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T15-bridges This PR/Issue is related to bridges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants