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

WithMaxSize #4188

Closed
wants to merge 6 commits into from
Closed

WithMaxSize #4188

wants to merge 6 commits into from

Conversation

xlc
Copy link
Contributor

@xlc xlc commented Apr 18, 2024

closes #4092
we may want to revert #3749

TODOs:

@@ -48,6 +48,7 @@ pub trait ExecuteControllerWeightInfo {

parameter_types! {
pub const MaxXcmEncodedSize: u32 = xcm::MAX_XCM_ENCODED_SIZE;
pub const MaxXcmEncodedSizeUsize: usize = xcm::MAX_XCM_ENCODED_SIZE as usize;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what's the best way to handle this

@xlc
Copy link
Contributor Author

xlc commented Apr 18, 2024

Screenshot 2024-04-18 at 10 01 17 PM

substrate/frame/support/src/utils.rs Outdated Show resolved Hide resolved
substrate/frame/support/src/utils.rs Outdated Show resolved Hide resolved
substrate/frame/support/src/utils.rs Outdated Show resolved Hide resolved
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5992305

@xlc
Copy link
Contributor Author

xlc commented Apr 19, 2024

Please let me know what should I do with the changes in pallet-xcm

  1. Only keep the WithMaxSize change in this PR and fix pallet-xcm in next PR.
  2. Fix send and execute and keep send_blob and execute_blob.
  3. Completely revert pallet-xcm: Deprecate execute and send in favor of execute_blob and send_blob #3749 and then fix send and execute

}

pub fn value(self) -> T {
self.value
Copy link
Member

Choose a reason for hiding this comment

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

Could make sense to impl the Deref trait and mark with WrapperTypeEncode and WrapperTypeDecode.

@xlc
Copy link
Contributor Author

xlc commented Apr 19, 2024

I guess @franciscoaguirre you are the right person to ping?

@acatangiu
Copy link
Contributor

@xlc let me verify this fully fixes the issue please before putting in more effort here, can't promise to do it today, but latest Monday

if into.len() > self.remaining_len {
return Err("Not enough data to fill buffer".into());
}
self.input.read(into)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
self.input.read(into)
let res = self.input.read(into)?;
self.remaining_len -= into.len();
Ok(res)

We actually need to remove it from the remaining length :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I really need some tests XD
and do we need to remove it before the read or it wouldn’t matter?

Copy link
Member

Choose a reason for hiding this comment

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

I thought about it. If this returns an error, I would assume that we bubble that up any way and the decoding failed.

@xlc
Copy link
Contributor Author

xlc commented Apr 23, 2024

lazy decode the Call will be a better way

@xlc xlc closed this Apr 23, 2024
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.

A way to annotate and enforce a max size of call parameters
5 participants