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

Adds Snowbridge to Kusama and Polkadot Runtimes #130

Merged

Conversation

claravanstaden
Copy link
Contributor

@claravanstaden claravanstaden commented Dec 22, 2023

cc @vgeddes @alistair-singh @yrong @musnit

@claravanstaden claravanstaden changed the title Adds Snowbridge (based on Kusama <-> Polkadot bridge PR) Draft - Adds Snowbridge (based on Kusama <-> Polkadot bridge PR) Dec 22, 2023
@claravanstaden claravanstaden mentioned this pull request Jan 15, 2024
@bkontur
Copy link
Contributor

bkontur commented Jan 23, 2024

There is bump to [email protected] with MessageQueues here,
So, I would maybe suggest rebasing this PR on the current main and merging #137 into this PR—it will bring us a little bit closer to the end :)

After #137 is merged, we just need to bump to [email protected].

@claravanstaden claravanstaden force-pushed the sv-pk-bridge-snowbridge branch 4 times, most recently from 2aff354 to 4fcef54 Compare January 26, 2024 13:21
@claravanstaden claravanstaden deleted the sv-pk-bridge-snowbridge branch February 13, 2024 11:24
@claravanstaden claravanstaden restored the sv-pk-bridge-snowbridge branch February 13, 2024 11:25
bkontur and others added 16 commits February 16, 2024 14:10
# Conflicts:
#	Cargo.lock
#	relay/kusama/constants/Cargo.toml
#	relay/polkadot/constants/Cargo.toml
#	system-parachains/asset-hubs/asset-hub-polkadot/src/xcm_config.rs
#	system-parachains/bridge-hubs/bridge-hub-kusama/tests/tests.rs
#	system-parachains/bridge-hubs/bridge-hub-polkadot/src/xcm_config.rs
# Conflicts:
#	Cargo.lock
#	relay/kusama/constants/Cargo.toml
#	relay/polkadot/constants/Cargo.toml
#	system-parachains/asset-hubs/asset-hub-kusama/src/lib.rs
#	system-parachains/asset-hubs/asset-hub-polkadot/src/lib.rs
claravanstaden and others added 18 commits March 19, 2024 12:25
# Conflicts:
#	CHANGELOG.md
#	integration-tests/emulated/chains/parachains/testing/penpal/Cargo.toml
#	system-parachains/asset-hubs/asset-hub-kusama/src/xcm_config.rs
#	system-parachains/asset-hubs/asset-hub-polkadot/src/lib.rs
#	system-parachains/asset-hubs/asset-hub-polkadot/src/xcm_config.rs
#	system-parachains/bridge-hubs/bridge-hub-kusama/src/lib.rs
#	system-parachains/bridge-hubs/bridge-hub-polkadot/src/lib.rs
…oreign_asset_pallet_deposit` to the AH runtimes
Nits for Snowbridge + stuff moving around
Copy link
Contributor

@bkontur bkontur left a comment

Choose a reason for hiding this comment

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

lgtm, just left few nits plus Snowfork#4

Comment on lines 807 to 809
/// User fee for ERC20 token transfer back to Ethereum.
/// Configure the fee to max Balance so that it is disabled.
/// Sensible value was 2_750_872_500_000.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not just about ERC20 token, this fee is applied whatever transfer we will do to the Ethereum with BridgeTable.
And all other tokens are disabled also, unless the governance sets the fee, right?
What does it mean "Sensible value was...."?

I would suggest to update this doc and also for AHP.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines +147 to +151
snowbridge_pallet_system::migration::v0::InitializeOnUpgrade<
Runtime,
ConstU32<BRIDGE_HUB_ID>,
ConstU32<ASSET_HUB_ID>,
>,
Copy link
Contributor

Choose a reason for hiding this comment

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

@claravanstaden Is this a one-time shot, or do we need it for every runtime upgrade? If it's the former, please move it before // permanent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -326,6 +328,21 @@ pub fn can_calculate_weight_for_paid_export_message_with_reserve_transfer() {
)
}

#[test]
fn change_ethereum_gateway_by_governance_works() {
Copy link
Contributor

Choose a reason for hiding this comment

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

@claravanstaden very nit, why not to move it to the tests/snowbridge.rs also for BHP

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@acatangiu
Copy link
Contributor

@claravanstaden I think this is ready to merge now, yes?

@claravanstaden
Copy link
Contributor Author

@acatangiu yes, thank you!

@acatangiu
Copy link
Contributor

/merge

@fellowship-merge-bot fellowship-merge-bot bot merged commit 14cd363 into polkadot-fellows:main Mar 21, 2024
33 of 34 checks passed
@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.

10 participants