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

Conversation

@gui1117
Copy link
Contributor

@gui1117 gui1117 commented Sep 10, 2021

some of the assertion weren't checking much, this is an improved version

);

// Ensure nothing except the storage_version_key is stored in the new prefix.
assert!(new_pallet_prefix_iter.all(|key| key == storage_version_key));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

in the past we only ensured that the first key was sotrage_version_key, now we ensure it is the only one.

Copy link
Contributor

@emostov emostov Sep 21, 2021

Choose a reason for hiding this comment

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

tiny nit, but this doesn't check length and at this point we only expect one key under the new prefix, right? Would it be more accurate to do:

assert!(new_pallet_prefix_iter.next(), Some(storage_version_key));
assert!(new_pallet_prefix_iter.next(), None);

?

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 it is true, but I was thinking if they change the name, maybe it is ok not to have anything stored at the new prefix.

old_pallet_prefix.to_vec(),
|_| Ok(()),
);
assert_eq!(old_pallet_prefix_iter.count(), 0);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is the past we only checked for the first key at the old prefix, and we accepted that it was the storage_version_key, now we ensure nothing remains there.

@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. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Sep 10, 2021
@gui1117
Copy link
Contributor Author

gui1117 commented Sep 21, 2021

bot merge

@ghost
Copy link

ghost commented Sep 21, 2021

Waiting for commit status.

@ghost
Copy link

ghost commented Sep 21, 2021

Merge aborted: Checks failed for bbb6039

@gui1117
Copy link
Contributor Author

gui1117 commented Sep 21, 2021

bot merge

@ghost
Copy link

ghost commented Sep 21, 2021

Error: Github API says #9746 is not mergeable

@gui1117
Copy link
Contributor Author

gui1117 commented Sep 21, 2021

bot merge

@ghost
Copy link

ghost commented Sep 21, 2021

Waiting for commit status.

@ghost
Copy link

ghost commented Sep 21, 2021

Merge aborted: Head SHA changed from 4a4a1d1 to 7ce0438

@gui1117
Copy link
Contributor Author

gui1117 commented Sep 21, 2021

bot merge

@ghost
Copy link

ghost commented Sep 21, 2021

Waiting for commit status.

@ghost ghost merged commit ce3c31f into master Sep 21, 2021
@ghost ghost deleted the gui-improve-memebership-post-migration branch September 21, 2021 12:36
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. 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.

4 participants