-
Notifications
You must be signed in to change notification settings - Fork 141
Fix and re-enable some xcm tests #1009
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: main
Are you sure you want to change the base?
Conversation
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.
Found a logic error due to swapped function arguments in two test files, which will likely cause the tests to fail. Also identified duplicated test logic that could be refactored.
| /// - Parachain should be able to create a new Foreign Asset at Asset Hub | ||
| #[test] | ||
| fn send_xcm_from_para_to_asset_hub_paying_fee_with_system_asset() { | ||
| pub fn penpal_register_foreign_asset_on_asset_hub(asset_location_on_penpal: Location) { |
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.
The function penpal_register_foreign_asset_on_asset_hub is nearly identical to the one added in integration-tests/emulated/tests/assets/asset-hub-polkadot/src/tests/send.rs. Consider moving this logic to a shared helper to avoid code duplication.
| let call = AssetHubKusama::create_foreign_asset_call( | ||
| foreign_asset_at_asset_hub.clone(), | ||
| ASSET_MIN_BALANCE, | ||
| para_sovereign_account, |
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.
The owner and min_balance arguments for create_foreign_asset_call appear to be swapped. ASSET_MIN_BALANCE is being passed as the owner, and para_sovereign_account is passed as the minimum balance. This will lead to a runtime error or incorrect behavior. They should be swapped to match the expected signature (id, owner, min_balance).
| let call = AssetHubPolkadot::create_foreign_asset_call( | ||
| foreign_asset_at_asset_hub.clone(), | ||
| ASSET_MIN_BALANCE, | ||
| para_sovereign_account, |
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.
The owner and min_balance arguments for create_foreign_asset_call appear to be swapped. ASSET_MIN_BALANCE is being passed as the owner, and para_sovereign_account is passed as the minimum balance. This will lead to a runtime error or incorrect behavior. They should be swapped to match the expected signature (id, owner, min_balance).
Fixed some broken and ignored XCM tests.