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

Export decode_vec_with_len #403

Merged
merged 3 commits into from
Jan 26, 2023
Merged

Conversation

mrcnski
Copy link
Contributor

@mrcnski mrcnski commented Jan 23, 2023

This is necessary to fix the custom impl Decode for BoundedVec. Currently when decoding, it will allocate before doing a length check. See:

paritytech/polkadot#6603 (comment)

You want to ensure that you don't even start try decoding/allocating when the length of the vector is more than the
allowed maximum.

You first decode length of the vector and then early reject if that is too long.

This is necessary to implement a custom `Decode` for `BoundedVec`. See:

paritytech/polkadot#6603 (comment)

> You want to ensure that you don't even start try decoding/allocating when the length of the vector is more than the
> allowed maximum.

> You first decode length of the vector and then early reject if that is too long.
@mrcnski mrcnski requested review from KiChjang and bkchr January 23, 2023 11:24
@bkchr
Copy link
Member

bkchr commented Jan 24, 2023

@mrcnski you also need to bump the patch version and then release this version!

@mrcnski
Copy link
Contributor Author

mrcnski commented Jan 24, 2023

@bkchr I'll need some help with that. Should the version bump be a separate PR? How do I release it? (I don't think I have access rights, or do I?)

@bkchr
Copy link
Member

bkchr commented Jan 24, 2023

You can bump the patch version in this pr. Then merge it. Then do cargo publish. Then go on github and create some new release. I can create the github release for you if you don't know how to.

@mrcnski
Copy link
Contributor Author

mrcnski commented Jan 24, 2023

Okay! Technically though, shouldn't this be a minor version bump as we are adding to the public API?

@bkchr
Copy link
Member

bkchr commented Jan 24, 2023

Technically though, shouldn't this be a minor version bump as we are adding to the public API?

Why not? We don't break anything? I mean we can also bump the minor version 🤷

@mrcnski
Copy link
Contributor Author

mrcnski commented Jan 24, 2023

@bkchr The major version is > 0 so a minor version bump doesn't denote a breaking change, it just means that functionality was added.

@bkchr
Copy link
Member

bkchr commented Jan 24, 2023

@bkchr The major version is > 0 so a minor version bump doesn't denote a breaking change, it just means that functionality was added.

I know.

I read again semver:

MINOR version when you add functionality in a backwards compatible manner
PATCH version when you make backwards compatible bug fixes

Let's bump the minor. I general before I meant that you can do whatever you want :D I would not have blocked you on bumping the minor :D

@mrcnski mrcnski merged commit 31078e5 into master Jan 26, 2023
@mrcnski
Copy link
Contributor Author

mrcnski commented Jan 26, 2023

Cool, I was able to cargo publish -- didn't know I could do that. Raised a new release as well!

@bkchr bkchr deleted the mrcnski/export-decode_vec_with_len branch January 26, 2023 12:58
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