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

Add XcmPaymentApi and DryRunApi to all runtimes #380

Merged

Conversation

franciscoaguirre
Copy link
Contributor

@franciscoaguirre franciscoaguirre commented Jul 12, 2024

Adds the paritytech/polkadot-sdk#3607 and paritytech/polkadot-sdk#3872 to all runtimes.
These APIs work together to allow dry running and estimating execution and delivery fees for XCMs.

This PR doesn't allow querying the price in assets different than the relay token for asset hubs. Can be done in a following PR.
Also tracking #388 and #389 for future improvemens.

Old PR was #359, this one targets main already updated to Polkadot SDK 1.13.

Dear reviewer

Although there are a lot of changes, the main one is the addition of the two aforementioned APIs to all relays and system parachains. The rest of the files are tests, which all have the same format.

@acatangiu
Copy link
Contributor

Why is it showing all of these commits? Can you try:

git pull upstream main --rebase
git push -f

so you rebase your commits on latest master, then this PR will show up clean?

@franciscoaguirre
Copy link
Contributor Author

@acatangiu I pulled main and merged, but it didn't work. So I just branched off of main and cherry-picked the relevant commits. It's all good now :)

@franciscoaguirre
Copy link
Contributor Author

Just missing the changelog

Copy link
Contributor

@acatangiu acatangiu left a comment

Choose a reason for hiding this comment

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

Please add a couple integration tests for them too (ideally at least one per chain) so we have basic coverage that they're working.

CHANGELOG.md Outdated Show resolved Hide resolved
@franciscoaguirre
Copy link
Contributor Author

@acatangiu I added a test for XCM fee estimation but for some reason the last hop of the message does not get executed. The InitiateReserveWithdraw in PenpalA happens, the DepositReserveAsset in AssetHubPolkadot happens, but then the ReserveAssetDeposited in PenpalB doesn't happen. Checked the logs and I don't see any error. I'll keep investigating.

@acatangiu
Copy link
Contributor

@acatangiu I added a test for XCM fee estimation but for some reason the last hop of the message does not get executed. The InitiateReserveWithdraw in PenpalA happens, the DepositReserveAsset in AssetHubPolkadot happens, but then the ReserveAssetDeposited in PenpalB doesn't happen. Checked the logs and I don't see any error. I'll keep investigating.

Make sure to "progress" the chains in the emulator - the chains only "make progress" (process messages, etc) when their externalities are instantiated. E.g. make sure for each chain to have some

Chain::execute_with(|| {
    // bla
});

@franciscoaguirre
Copy link
Contributor Author

I added tests for AssetHub Polkadot and Kusama. For BridgeHub I'm thinking the test should estimate fees over the bridge and for the rest of the chains I just want to estimate teleports from AssetHub to them.

@acatangiu
Copy link
Contributor

I added tests for AssetHub Polkadot and Kusama. For BridgeHub I'm thinking the test should estimate fees over the bridge and for the rest of the chains I just want to estimate teleports from AssetHub to them.

You could add the estimation here and here.

Then use these macros for all system chains and get full relay<>system-chains coverage.

(You should add it in separate PR to SDK too: https://github.com/paritytech/polkadot-sdk/blob/291210aa0fafa97d9b924fe82d68c023bdb0a340/cumulus/parachains/integration-tests/emulated/common/src/macros.rs#L42)

@franciscoaguirre
Copy link
Contributor Author

Integrated the delivery fees into both test_relay_is_trusted_teleporter and test_parachain_is_trusted_teleporter_for_relay.
They are only being used by collectives-polkadot. I'll add them in all other runtimes, including BridgeHub, that one will also get its own "over the bridge" test.

@franciscoaguirre
Copy link
Contributor Author

franciscoaguirre commented Jul 16, 2024

I saw PeopleKusama, PeoplePolkadot and Encointer don't have price for parent delivery set up. It's probably a misstep. I'll add it on this PR. Is there a reason for this @acatangiu ?

@franciscoaguirre
Copy link
Contributor Author

Just missing adding some delivery fee estimation in an asset transfer over the bridge and this is good to go 🚀

@franciscoaguirre
Copy link
Contributor Author

Hmm, the bridge fee estimation has some qwirks. You think we can merge this as is and I'll open another PR with an example of bridge estimation? @acatangiu

@acatangiu
Copy link
Contributor

Hmm, the bridge fee estimation has some qwirks. You think we can merge this as is and I'll open another PR with an example of bridge estimation? @acatangiu

Sure. I opened #388 and #389 to not lose track of them.

fellowship-merge-bot bot pushed a commit that referenced this pull request Jul 18, 2024
Fixes #388.

While working on #380 I
noticed `Encointer`, `PeopleKusama` and `PeoplePolkadot` didn't have
delivery fees setup for UMP.
In most cases, the types were created but not used.
This PR uses them.

Choose a reason for hiding this comment

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

// We need to mint funds into the checking account of `$receiver_relay`
// for it to accept a teleport from `$sender_para`.
// Else we'd get a `NotWithdrawable` error since it tries to reduce the check account balance, which
// would be 0.
<$receiver_relay>::execute_with(|| {
    let check_account = <$receiver_relay as [<$receiver_relay Pallet>]>::XcmPallet::check_account();
    assert_ok!(<$receiver_relay as [<$receiver_relay Pallet>]>::Balances::mint_into(
	    &check_account,
	    $amount,
    ));
});

In test_parachain_is_trusted_teleporter_for_relay, I am confused that why relay's check account balance would be reduced when relay is receiver

Copy link
Contributor

Choose a reason for hiding this comment

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

The checking account on Relay tracks how many DOTs have been teleported out. So when they are teleported back in, this number decreases (is reduced).

@franciscoaguirre
Copy link
Contributor Author

/merge

@fellowship-merge-bot fellowship-merge-bot bot enabled auto-merge (squash) July 22, 2024 10:31
@fellowship-merge-bot
Copy link
Contributor

Enabled auto-merge in Pull Request

Available commands
  • /merge: Enables auto-merge for Pull Request
  • /merge cancel: Cancels auto-merge for Pull Request
  • /merge help: Shows this menu

For more information see the documentation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants