Skip to content

Fix max_encoded_len for Compact fields#508

Merged
bkchr merged 6 commits intomasterfrom
pg/max-encoded-len
Sep 4, 2023
Merged

Fix max_encoded_len for Compact fields#508
bkchr merged 6 commits intomasterfrom
pg/max-encoded-len

Conversation

@pgherveou
Copy link
Copy Markdown
Contributor

@pgherveou pgherveou commented Sep 1, 2023 •
edited
Loading

Noticed that when using compact fields the max_encoded_len macro expand to the same as if we were not using the compact attribute.

Screenshot 2023-09-01 at 21 44 55

#[derive(Encode, Decode, MaxEncodedLen)]
pub struct Weight {
    #[codec(compact)]
    ref_time: u64,
    #[codec(compact)]
    proof_size: u64,
}

Will expand to

    impl ::codec::MaxEncodedLen for Weight {
        fn max_encoded_len() -> ::core::primitive::usize {
            0_usize
                .saturating_add(<u64>::max_encoded_len())
                .saturating_add(<u64>::max_encoded_len())
        }
    }

This PR fix it to

    impl ::codec::MaxEncodedLen for Weight {
        fn max_encoded_len() -> ::core::primitive::usize {
            0_usize
                .saturating_add(::parity_scale_codec::Compact<u64>::max_encoded_len())
                .saturating_add(::parity_scale_codec::Compact<u64>::max_encoded_len())
        }
    }

@pgherveou pgherveou marked this pull request as ready for review September 1, 2023 21:21
@pgherveou pgherveou requested a review from KiChjang September 1, 2023 21:24
KiChjang
KiChjang reviewed Sep 2, 2023
View reviewed changes
KiChjang
KiChjang reviewed Sep 2, 2023
View reviewed changes
@pgherveou pgherveou requested a review from KiChjang September 2, 2023 04:44
@pgherveou
Copy link
Copy Markdown
Contributor Author

pgherveou commented Sep 2, 2023

also a few side thoughts:
Do we really need saturating_add for implementing this trait?
Could this be a candidate for const fn?

@bkchr
Copy link
Copy Markdown
Member

bkchr commented Sep 4, 2023

also a few side thoughts: Do we really need saturating_add for implementing this trait?

I mean we probably never use more than usize, but there is also no harm in using saturating_add.

Could this be a candidate for const fn?

const fn trait functions are not supported right now.

bkchr
bkchr approved these changes Sep 4, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Some nitpicks, otherwise looks good 👍

Good work :)

pgherveou and others added 2 commits September 4, 2023 07:28
@pgherveou @bkchr
Apply suggestions from code review
dbb2e5f 
Co-authored-by: Bastian Köcher <git@kchr.de>
@pgherveou
fix ui-test output
281165d
@bkchr bkchr merged commit ddf9439 into master Sep 4, 2023
@bkchr bkchr deleted the pg/max-encoded-len branch September 4, 2023 07:16
@bkchr
Copy link
Copy Markdown
Member

bkchr commented Sep 4, 2023

@pgherveou could you release v3.6.5?

@pgherveou
Copy link
Copy Markdown
Contributor Author

pgherveou commented Sep 4, 2023

isnt 3.6.5 the current version, you mean 3.6.6?

This was referenced Sep 6, 2023
Merged
Open
ascjones added a commit that referenced this pull request Nov 29, 2023
@ascjones
Revert "Fix max_encoded_len for Compact fields (#508)"
39058e4 
This reverts commit ddf9439
@ascjones ascjones mentioned this pull request Nov 29, 2023
Merged
ascjones added a commit that referenced this pull request Nov 29, 2023
@ascjones
Revert breaking changes, bump version to 3.6.9 (#545)
d82f943 
* Revert "Update MaxEncodedLen derive macro (#512)"

This reverts commit 1e3f80c.

* Revert "Fix max_encoded_len for Compact fields (#508)"

This reverts commit ddf9439

* Bump version to 3.6.9

* Cargo.lock

* ui test
jsdw added a commit that referenced this pull request Dec 2, 2024
@jsdw
Revert "Fix max_encoded_len for Compact fields (#508)"
d4c75ae 
This reverts commit ddf9439.
serban300 added a commit that referenced this pull request Jan 23, 2025
@jsdw @serban300
Revert #512 and #508 (#663)
96269d4 
* Revert "Update MaxEncodedLen derive macro (#512)"

This reverts commit 1e3f80c.

* Revert "Fix max_encoded_len for Compact fields (#508)"

This reverts commit ddf9439.

---------

Co-authored-by: Serban Iorga <serban@parity.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bkchr bkchr bkchr approved these changes

@KiChjang KiChjang Awaiting requested review from KiChjang

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@pgherveou @bkchr @KiChjang