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

Limit Call size to 200 bytes #598

Merged
merged 13 commits into from
Sep 2, 2021

Conversation

ferrell-code
Copy link
Contributor

@ferrell-code ferrell-code commented Sep 1, 2021

The idea of this PR is to limit call length to 200 bytes for all orml pallets. It mirrors paritytech/substrate#9418

Companion to AcalaNetwork/Acala#1386

@ferrell-code ferrell-code marked this pull request as draft September 1, 2021 22:35
@xlc xlc requested a review from shaunxw September 1, 2021 22:37
@codecov-commenter
Copy link

codecov-commenter commented Sep 1, 2021

Codecov Report

Merging #598 (33bc599) into master (74d290d) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #598      +/-   ##
==========================================
+ Coverage   75.54%   75.58%   +0.04%     
==========================================
  Files          76       76              
  Lines        5945     5955      +10     
==========================================
+ Hits         4491     4501      +10     
  Misses       1454     1454              
Impacted Files Coverage Δ
authority/src/lib.rs 85.29% <100.00%> (ø)
authority/src/tests.rs 100.00% <100.00%> (ø)
xcm/src/lib.rs 66.66% <100.00%> (ø)
xtokens/src/lib.rs 71.91% <100.00%> (ø)
xtokens/src/tests.rs 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 74d290d...33bc599. Read the comment docs.

Comment on lines +393 to +401

#[test]
fn call_size_limit() {
assert!(
core::mem::size_of::<authority::Call::<Runtime>>() <= 200,
"size of Call is more than 200 bytes: some calls have too big arguments, use Box to \
reduce the size of Call.
If the limit is too strong, maybe consider increasing the limit",
);
Copy link
Contributor Author

@ferrell-code ferrell-code Sep 2, 2021

Choose a reason for hiding this comment

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

This tests passes, yet in karura runtime the Call size for orml_authority is printed as 608 bytes, this test when printed says call size of 56 bytes. That doesn't really make sense to me

Edit: Nvm, I think I figured it out it probably is because karura sets something different in the runtime than the mock 👍

@ferrell-code ferrell-code marked this pull request as ready for review September 2, 2021 04:14
@ferrell-code
Copy link
Contributor Author

ferrell-code commented Sep 2, 2021

I could add a integration test that checks every pallet, although there will be a test in Karura/mandala that would catch any large Calls.

It seems that the XCM stuff has some enums that allocate (relatively) a lot memory as arguments for the extrinsics so I left tests checking those pallets

initial_origin: T::PalletsOrigin,
initial_origin: Box<T::PalletsOrigin>,
Copy link
Contributor Author

@ferrell-code ferrell-code Sep 2, 2021

Choose a reason for hiding this comment

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

So boxing T::PalletsOrigin decreases the call size for Authority from 608 bytes to 24 bytes in karura. Not entirely sure why but numbers don't lie

Copy link
Member

@shaunxw shaunxw left a comment

Choose a reason for hiding this comment

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

LGTM

@xlc xlc merged commit ffa9c5f into open-web3-stack:master Sep 2, 2021
@shaunxw
Copy link
Member

shaunxw commented Sep 2, 2021

Yeah it would be helpful to add integration tests. If there're generic typed params in dispatchable calls, call size would likely to be different between mocks and prod runtimes.

@ferrell-code ferrell-code deleted the fer-smaller-call-sizes branch September 2, 2021 22:12
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.

4 participants