Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Fix WrapperOpaque max encded len and type info #9881

Merged
6 commits merged into from
Sep 29, 2021
Merged

Conversation

gui1117
Copy link
Contributor

@gui1117 gui1117 commented Sep 28, 2021

No description provided.

@gui1117 gui1117 mentioned this pull request Sep 28, 2021
2 tasks
@gui1117 gui1117 added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Sep 28, 2021
@gui1117
Copy link
Contributor Author

gui1117 commented Sep 28, 2021

@ascjones maybe you can review this one ? I forgot to declare the correct type info of the type

@gui1117 gui1117 added C3-medium PR touches the given topic and has a medium impact on builders. and removed B0-silent Changes should not be mentioned in any release notes labels Sep 28, 2021
@gui1117 gui1117 added B0-silent Changes should not be mentioned in any release notes and removed C1-low PR touches the given topic and has a low impact on builders. labels Sep 28, 2021
Comment on lines 430 to 431
// Note: this can be improved if `T::max_encoded_len` is small.
// E.g. if T max encoded len is 4, then the compact encoding of its encoded length is 1.
Copy link
Member

Choose a reason for hiding this comment

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

Why this comment here?

The point of this is to return the max encoded length?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes but I think we can be more precise in certain situation, I finally did the more precise implementation 5f07186

@gui1117
Copy link
Contributor Author

gui1117 commented Sep 29, 2021

bot merge

@ghost
Copy link

ghost commented Sep 29, 2021

Trying merge.

@ghost ghost merged commit 95c81d2 into master Sep 29, 2021
@ghost ghost deleted the gui-fix-wrapper-opaque branch September 29, 2021 07:48
@gui1117
Copy link
Contributor Author

gui1117 commented Sep 29, 2021

ah sorry I didn't see you asked review of ascjones, I'll ping him and ensure he get to see it

Copy link
Contributor

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

LGTM. This type is potentially useful and universal enough to promote to parity-scale-codec and scale-info as a built-in.

.type_params(vec![TypeParameter::new("T", Some(meta_type::<T>()))])
.composite(
Fields::unnamed()
.field(|f| f.compact::<u32>().type_name("EncodedLengthOfT"))
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need type_name here. It is optional and is used for the name of the type of the field as it appears in the code, in order to identify alias usage. E.g. where the concrete type is u128 the type_name would be Balance for example.

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 see, I'll do another PR with this removed

This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C3-medium PR touches the given topic and has a medium impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants