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

Transact without specifying weight #6228

Merged
merged 11 commits into from
Oct 29, 2024
Merged

Conversation

franciscoaguirre
Copy link
Contributor

@franciscoaguirre franciscoaguirre commented Oct 24, 2024

Addresses #4284

For V5, removed required_weight_at_most from Transact.
The weigher now has to decode the call inside of a transaction in order to know it's dispatch weight.
It's harder to make mistakes now, since the user no longer specifies a weight value which might be wrong.

@franciscoaguirre franciscoaguirre requested review from a team as code owners October 24, 2024 19:31
@paritytech-review-bot paritytech-review-bot bot requested a review from a team October 24, 2024 19:32
@acatangiu acatangiu added the T6-XCM This PR/Issue is related to XCM. label Oct 25, 2024
@acatangiu acatangiu changed the base branch from master to xcm-v5 October 25, 2024 13:29
@acatangiu acatangiu mentioned this pull request Oct 25, 2024
11 tasks
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.

Nice!

polkadot/runtime/westend/src/lib.rs Show resolved Hide resolved
@@ -94,7 +94,7 @@ use sp_staking::SessionIndex;
#[cfg(any(feature = "std", test))]
use sp_version::NativeVersion;
use sp_version::RuntimeVersion;
use xcm::v4::{Assets, InteriorLocation, Location, SendError, SendResult, SendXcm, XcmHash};
use xcm::v5::{Assets, InteriorLocation, Location, SendError, SendResult, SendXcm, XcmHash};
Copy link
Contributor

Choose a reason for hiding this comment

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

@franciscoaguirre yes, yes, my favorite, I think we should use xcm::latest here :D

@seadanda I also think we should refactor polkadot/runtime/parachains/src/coretime/mod.rs to use xcm::latest instead of the specific xcmV4 version. If I understand correctly, this pallet only sends some XCM at some point and does not store XCM data/structs, so it should be fine and preferable to use xcm::latest.

Keeping this pallet on xcmV4 while migrating the rest to xcmV5 could lead to potential issues, as we would need to manage multiple imports in the fellows' runtimes, e.g., xcm::v4::SendXcm and xcm::v5::SendXcm.

For now, it’s fine on the master since xcmV4 is the latest version. However, I recommend refactoring the coretime pallet directly on the master branch before merging xcmV5.

polkadot/xcm/src/v4/mod.rs Outdated Show resolved Hide resolved
"Max weight bigger than require at most",
);

return Err(XcmError::MaxWeightInvalid)
Copy link
Contributor

Choose a reason for hiding this comment

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

haha, now we could remove MaxWeightInvalid from the xcm spec - related to the #6199
(no need to do anything here, just a comment :))

@franciscoaguirre franciscoaguirre force-pushed the transact-without-specifying-weight branch from 0fc5cc8 to a044c7c Compare October 28, 2024 21:00
@franciscoaguirre
Copy link
Contributor Author

bot fmt

@command-bot
Copy link

command-bot bot commented Oct 28, 2024

@franciscoaguirre https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7657414 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 5-362fa557-75ae-4470-96a8-d0294355ceb1 to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Oct 28, 2024

@franciscoaguirre Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7657414 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7657414/artifacts/download.

@franciscoaguirre
Copy link
Contributor Author

@bkontur Updated with feedback

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.

nice, lgtm

@acatangiu acatangiu merged commit a1b8381 into xcm-v5 Oct 29, 2024
175 of 198 checks passed
@acatangiu acatangiu deleted the transact-without-specifying-weight branch October 29, 2024 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T6-XCM This PR/Issue is related to XCM.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants