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

Backport claim assets tests from polkadot-fellows repo #4930

Conversation

programskillforverification
Copy link
Contributor

@programskillforverification programskillforverification commented Jul 2, 2024

Issue

Backport integration tests for claim assets to the polkadot-sdk from polkadot-fellows repo #4892

Description

For the first time to contribute this project, I just finished part and make sure everything is ok.

@cla-bot-2021
Copy link

cla-bot-2021 bot commented Jul 2, 2024

User @programskillforverification, please sign the CLA here.

@programskillforverification programskillforverification marked this pull request as ready for review July 3, 2024 03:18
@programskillforverification programskillforverification marked this pull request as draft July 3, 2024 03:23
@bkontur bkontur self-requested a review July 3, 2024 08:02
@bkontur
Copy link
Contributor

bkontur commented Jul 4, 2024

bot fmt

@programskillforverification programskillforverification marked this pull request as ready for review July 5, 2024 10:48
@programskillforverification programskillforverification requested a review from a team as a code owner July 5, 2024 10:48
Copy link
Contributor

@franciscoaguirre franciscoaguirre left a comment

Choose a reason for hiding this comment

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

We probably want the test for claiming assets on all runtimes, thinking people chain and coretime chain for sure

@paritytech-review-bot paritytech-review-bot bot requested a review from a team July 9, 2024 10:01
@programskillforverification
Copy link
Contributor Author

We probably want the test for claiming assets on all runtimes, thinking people chain and coretime chain for sure

@franciscoaguirre Okay, claiming assets will be added to people chain. But there is no any tests about coretime chain in whole integration tests directory. Are you sure to import it?

@programskillforverification programskillforverification marked this pull request as draft July 9, 2024 10:41
@acatangiu
Copy link
Contributor

But there is no any tests about coretime chain in whole integration tests directory. Are you sure to import it?

Yes. We should add emulated tests for coretime chain as well.

@programskillforverification
Copy link
Contributor Author

I just add tests in rococo. If everything is ok, especially config, I will add them in westend.

@programskillforverification programskillforverification marked this pull request as ready for review July 11, 2024 13:56
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.

very nice, thanks @programskillforverification !

everything looks good, feel free to do the same for Westend.

Could you please also include the teleport tests for all chains that don't have it? (e.g. coretime emulated chains you just added)

Cargo.toml Outdated Show resolved Hide resolved
substrate/frame/bags-list/src/list/tests.rs Outdated Show resolved Hide resolved
substrate/frame/nis/src/lib.rs Outdated Show resolved Hide resolved
@programskillforverification
Copy link
Contributor Author

very nice, thanks @programskillforverification !

everything looks good, feel free to do the same for Westend.

Could you please also include the teleport tests for all chains that don't have it? (e.g. coretime emulated chains you just added)

What should the teleport tests include in coretime? Teleport tests looks different for different chains.

@acatangiu
Copy link
Contributor

What should the teleport tests include in coretime? Teleport tests looks different for different chains.

Basic ROC (on Rococo) and WND (on Westend) teleport tests: between Coretime and Relay and between coretime and other system chains.

(this ^ should be minimum for all system chains)

@programskillforverification
Copy link
Contributor Author

What should the teleport tests include in coretime? Teleport tests looks different for different chains.

Basic ROC (on Rococo) and WND (on Westend) teleport tests: between Coretime and Relay and between coretime and other system chains.

(this ^ should be minimum for all system chains)

Now, macro test_relay_is_trusted_teleporter! is already added.

/// Limited Teleport of native asset from Relay Chain to the System Parachain should work
#[test]
fn limited_teleport_native_assets_from_relay_to_system_para_works() {
	// Init values for Relay Chain
	let amount_to_send: Balance = WESTEND_ED * 1000;
	let dest = Westend::child_location_of(AssetHubWestend::para_id());
	let beneficiary_id = AssetHubWestendReceiver::get();
	let test_args = TestContext {
		sender: WestendSender::get(),
		receiver: AssetHubWestendReceiver::get(),
		args: TestArgs::new_relay(dest, beneficiary_id, amount_to_send),
	};

	let mut test = RelayToSystemParaTest::new(test_args);
...

Is this test function outdated? If so, I want to rewrite all teleport tests using macro.

@acatangiu
Copy link
Contributor

Yes, limited_teleport_native_assets_from_relay_to_system_para_works can be replaced by macro version - it does the same thing.

@programskillforverification programskillforverification changed the title [1/2]Backport claim assets tests from polkadot-fellows repo Backport claim assets tests from polkadot-fellows repo Jul 16, 2024
@programskillforverification
Copy link
Contributor Author

programskillforverification commented Jul 16, 2024

@franciscoaguirre PTAL

@programskillforverification are you part of Polkadot Fellowship? If not, please add a Polkadot address so we can provide a tip for this useful work 👍

Thanks a lot. I already add my address in my bio and submit a fellowship application.

@bkontur
Copy link
Contributor

bkontur commented Jul 18, 2024

I think we should also reflect here upcoming changes for dry-run api from this PR: https://github.com/polkadot-fellows/runtimes/pull/380/files#diff-7346706278168c97dfec9e73bec1a115274a2be89a3147b914dd2eedec9f6269R57

@programskillforverification
Copy link
Contributor Author

I think we should also reflect here upcoming changes for dry-run api from this PR: https://github.com/polkadot-fellows/runtimes/pull/380/files#diff-7346706278168c97dfec9e73bec1a115274a2be89a3147b914dd2eedec9f6269R57

I am refactoring teleport in a new PR, which covers some part of this PR. For XcmPaymentApi and DryRunApi, maybe are in separate PR.

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.

Changes are good, you just have to make the CI happy - you can check the failed checks on their details button to find out what/why's failing

@github-actions github-actions bot requested a review from acatangiu July 19, 2024 10:32
@programskillforverification
Copy link
Contributor Author

Changes are good, you just have to make the CI happy - you can check the failed checks on their details button to find out what/why's failing

I fixed most except check-semver. The rust version need to be upgraded in check-semver to run it.

@acatangiu
Copy link
Contributor

@bkontur @franciscoaguirre please review

Copy link
Contributor

@franciscoaguirre franciscoaguirre left a comment

Choose a reason for hiding this comment

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

Thank you for this effort!

prdoc/pr_4930.prdoc Outdated Show resolved Hide resolved
prdoc/pr_4930.prdoc Show resolved Hide resolved
Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com>
@acatangiu acatangiu enabled auto-merge July 19, 2024 14:37
@acatangiu acatangiu added this pull request to the merge queue Jul 19, 2024
Merged via the queue into paritytech:master with commit d16af0b Jul 19, 2024
157 of 160 checks passed
github-merge-queue bot pushed a commit that referenced this pull request Jul 26, 2024
# Description
This is continue of the work to backport
`emulated-integration-tests-common`, if you want to understand the full
context start with reading
[#4930](#4930)
TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this pull request Aug 2, 2024
# Issue
[Backport integration tests for claim assets to the polkadot-sdk from
polkadot-fellows repo
paritytech#4892](paritytech#4892)
# Description
For the first time to contribute this project, I just finished part and
make sure everything is ok.
- [x] backport [claim_assets test
case](https://github.com/polkadot-fellows/runtimes/blob/main/integration-tests/emulated/tests/assets/asset-hub-kusama/src/tests/claim_assets.rs)
to the polkadot-sdk testnet integration tests
- [x] backport macro
[test_chain_can_claim_assets](https://github.com/polkadot-fellows/runtimes/blob/main/integration-tests/emulated/helpers/src/lib.rs#L218)
from fellows repo
- [ ] when merged to polkadot-sdk and released, make sure that it is
propagated to the fellows repo:
polkadot-fellows/runtimes#363
- [x] backport and align other macros/test-cases from
https://github.com/polkadot-fellows/runtimes/blob/8ec28f96eeb30fbba30d29006d75e1a3fa1cea1c/integration-tests/emulated/helpers/src/lib.rs#L31-L33

---------

Co-authored-by: Zihan Zhao <josephzhao@Zihans-MacBook-Pro-3.local>
Co-authored-by: command-bot <>
Co-authored-by: Adrian Catangiu <adrian@parity.io>
Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com>
TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this pull request Aug 2, 2024
…5100)

# Description
This is continue of the work to backport
`emulated-integration-tests-common`, if you want to understand the full
context start with reading
[paritytech#4930](paritytech#4930)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T10-tests This PR/Issue is related to tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants