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

Ensure data size of identity pallet is bounded#9168

Merged
19 commits merged into
masterfrom
kckyeung/bounded-identity
Jul 8, 2021
Merged

Ensure data size of identity pallet is bounded#9168
19 commits merged into
masterfrom
kckyeung/bounded-identity

Conversation

@KiChjang

Copy link
Copy Markdown
Contributor

@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. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Jun 22, 2021
Comment thread frame/identity/src/lib.rs Outdated
Comment thread frame/identity/src/lib.rs Outdated
Comment thread frame/identity/src/lib.rs Outdated
display: Data::Raw(b"ten".to_vec()),
legal: Data::Raw(b"The Right Ordinal Ten, Esq.".to_vec()),
display: Data::Raw(b"ten".to_vec().try_into().unwrap()),
legal: Data::Raw(b"The Right Ordinal Ten, Esq.".to_vec().try_into().unwrap()),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤣

fn setting_subaccounts_should_work() {
new_test_ext().execute_with(|| {
let mut subs = vec![(20, Data::Raw(vec![40; 1]))];
let mut subs = vec![(20, Data::Raw(vec![40; 1].try_into().unwrap()))];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm seeing a lot of changes with this pattern. Maybe a test-scoped function like

fn one_raw_data<S>(n: u8) -> Data {
  Data::Raw(vec![n; 1].try_into().unwrap())
}

would be convenient?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the other alternative is to turn the other side into vec, or by creating an implementation of PartialEq of boundedvec and vec?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Implementing PartialEq actually makes a lot more sense, I'll do that instead.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok, this actually doesn't work, because the problem isn't so much that PartialEq wasn't implemented between Vec and BoundedVec, it's that Data::Raw accepts a BoundedVec instead of a Vec as its inner struct.

}

/// Exactly the same semantics as [`Vec::get_mut`].
pub fn get_mut<I: SliceIndex<[T]>>(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There should also be the same addition to the weakly_bounded_vec definition

Comment thread frame/identity/src/types.rs Outdated
Comment thread frame/identity/src/types.rs Outdated
Comment thread frame/identity/src/types.rs Outdated
@KiChjang KiChjang force-pushed the kckyeung/bounded-identity branch from e309366 to be9954b Compare July 6, 2021 05:03

@shawntabrizi shawntabrizi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

needs change below

Comment thread frame/identity/src/lib.rs Outdated
@shawntabrizi shawntabrizi self-requested a review July 7, 2021 01:23

@shawntabrizi shawntabrizi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

verify first, write last rule is not enforced

@shawntabrizi

Copy link
Copy Markdown
Member

bot merge

@ghost

ghost commented Jul 8, 2021

Copy link
Copy Markdown

Trying merge.

@ghost ghost merged commit 9235309 into master Jul 8, 2021
@ghost ghost deleted the kckyeung/bounded-identity branch July 8, 2021 02:57
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 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants