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

Conversation

@KiChjang
Copy link
Contributor

This will be very useful in structs containing BoundedVec,
where the second type parameter does not need to have any
trait bounds, since it represents the max supported length
of the BoundedVec.

This will be very useful in structs containing `BoundedVec`,
where the second type parameter does not need to have any
trait bounds, since it represents the max supported length
of the `BoundedVec`.
@KiChjang KiChjang 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. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Jun 23, 2021
@KiChjang KiChjang requested review from bkchr, coriolinus and gui1117 June 23, 2021 01:06
Copy link
Contributor

@coriolinus coriolinus left a comment

Choose a reason for hiding this comment

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

I think that NoBound is precisely the wrong handle the generic bounds for Bounded{Vec, BTreeSet, BTreeMap}. For ease of implementation, it's throwing away data that it can use to perform a precise calculation.

I.e. if we did

#[derive(Encode, Decode, MaxEncodedLenNoBound)
struct BoundedVec<T, S>(Vec<T>, PhantomData<S>);

Then what I'd expect is that BoundedVec<u8>::max_encoded_len() would produce a compile error: Vec<T> doesn't have a maximum encoded length.

What I'd expect instead is an implementation, whether manual or macro, like this:

impl<T: MaxEncodedLen, S: Get<u32>> MaxEncodedLen for BoundedVec<T, S> {
  fn max_encoded_len() -> usize {
    T::max_encoded_len().saturating_mul(Self::bound()).saturating_add(u32::max_encoded_len())
  }
}

The above could be pretty easily be made into a declarative macro; I think this extension to the proc macro isn't worth it.


The other factor in play right now is that we are currently in the process of moving the MaxEncodedLen implementation from this repo to parity-scale-codec: paritytech/parity-scale-codec#268, #9163. It would be better to wait until that move is complete before substantially editing the trait or macro.

#[cfg(feature = "derive")]
pub use max_encoded_len_derive::MaxEncodedLen;

/// Same as `MaxEncodedLen`, but without bounding the generics on the derived type with
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Same as `MaxEncodedLen`, but without bounding the generics on the derived type with
/// Same as [`MaxEncodedLen`](macro@MaxEncodedLen), but without bounding the generics on the derived type with

@bkchr
Copy link
Member

bkchr commented Jun 23, 2021

Yeah I see this as @coriolinus.

If you don't have the bound for T: MaxEcnodedLen, it would fail to compile. Besides that, we also control this macro. This means we can write custom bounds. Like for Encode with codec(encode_bound()) and we would like to have the same here.

So, if we need this, it needs to be implemented in the scale-codec crate after the mentioned pr is merged.

@bkchr bkchr closed this Jun 23, 2021
@gui1117
Copy link
Contributor

gui1117 commented Jun 23, 2021

maybe the macro MaxEncodedLen could handle custom bounds similar to this paritytech/parity-scale-codec#263

EDIT: I was too late :-)

@bkchr bkchr deleted the kckyeung/max-encoded-len-no-bound branch June 23, 2021 07:30
@bkchr
Copy link
Member

bkchr commented Jun 23, 2021

maybe the macro MaxEncodedLen could handle custom bounds similar to this paritytech/parity-scale-codec#263

Yeah, that is what I meant above :)

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 C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants