-
Notifications
You must be signed in to change notification settings - Fork 78
chore: AH Migration backport for Kintsugi #1253
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
base: kintsugi-1.25
Are you sure you want to change the base?
Conversation
…ing migration; test: tests for all the cases
|
Changed the base to |
| let bob_balance = kusama_runtime::Balances::free_balance(&AccountId::from(BOB)); | ||
|
|
||
| // A little bit less to pay fees | ||
| assert!(bob_balance < KSM.one()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should check that it's >0, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, I've modified the test
| let bob_balance = kusama_runtime::Balances::free_balance(&AccountId::from(BOB)); | ||
|
|
||
| // A little bit less to pay fees | ||
| assert!(bob_balance < 2 * KSM.one()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should check > 1 KSM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll be pushing right now the corrected tests
| let bob_balance = kusama_runtime::Balances::free_balance(&AccountId::from(BOB)); | ||
|
|
||
| // A little bit less to pay fees | ||
| assert!(bob_balance < KSM.one()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and >0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this one, it took us a lot of time cause we saw that the received balance was indeed 0. I was checking your repo and didn't see any test for pallet_xcm transfers, so I created one agains the 'kintsugi-1.25' branch and observed that the tokens weren't received on KSM even with your current runtime.
So, I guess this is something that should be fixed on your pallet_xcm configuration in the future. For now, it's not problematic because pallet-xcm reserve transfers will be disabled from the migration onwards, you can keep using xtokens for the moment, it's working properly.
I'm just getting rid of this test for this PR as it's useless
error: older versions of the `wasm-bindgen` crate are incompatible with current versions of Rust; please update to `wasm-bindgen` v0.2.8
This PR aims to protect Kintsugi's cross chain transfers from the upcoming AHM. During the migration, the inference of KSM reserves may lead to lost of funds, so we need to deactivate those transfers during the process.
A bit more context about this issue: https://forum.polkadot.network/t/mandatory-action-guide-for-ahm-broken-native-crosschain-transfers/14634
The proposed solution follows the approach of the ORML team implemented here: open-web3-stack/open-runtime-module-library#1033.
The affected extrinsics in Kintsugi are:
xtokens-> all extrinsics.xtokensuse aReserveProviderto determine the reserve, so the patch is needed here.pallet_xcm-> This old xcm version doesn't automatically infer the reserves, so theoretically it's not affected. However, theReserveProviderdefined onXcmExecutorisn't being taken into account by the extrinsics belonging to this pallet for some reason I couldn't figure out, but probably related to the old xcm version. I'm afraid that they keep using Kusama as the reserve after the migration which will result on a conflict with thextokensreserve (AssetHub). This might end up creating unexpected issues, so for the sake of security I've disabled transferring KSM usingpallet_xcmduring and after the migration. If the codebase is updated in the future to newer xcm versions this block might be removed, in the meanwhile, please usextokensto transfer KSM.Please note that all other reserves aren't affected by this patch and keep working as usual. The migration status should be changed by calling the
xtokens.set_migration_phaseextrinsic. This extrinsic is only callable by the root origin on Kintsugi, let us know if you prefer to use a different origin.As the Interbtc codebase deps are too far in the past, the time doesn't allow to bump everything and bring that change with the ORML crates, so we'll be using an ORML fork from the exact commit used by the Interbtc codebase. The fork is located in the R0gue GitHub organization, concretely here: https://github.com/r0gue-io/open-runtime-module-library/tree/master.
IMPORTANT: While this branch contains the patch for both Interlay and Kintsugi (just for convenience, to allow us compiling the whole workspace), it should only be used on Kintsugi yet. The branch sets off from the
1.25.6tag, so it's not suitable for an Interlay upgrade. However, backporting the patch should be straightforward.Note: I'm pushing this branch against the master branch. I'm not aware of Interbtc upgrades policies, so let me know if I should push it against another branch