Skip to content

fix: bump ProposedUpdatesEntries to V3 and clean up V2 #1665

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.

Sign up for GitHub

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

Jump to bottom
Merged
pbeza merged 3 commits into from
Dec 16, 2025
Merged

fix: bump ProposedUpdatesEntries to V3 and clean up V2 #1665

pbeza merged 3 commits into from
Dec 16, 2025

Conversation

@pbeza
Copy link
Contributor

@pbeza pbeza commented Dec 15, 2025

Fixes #1664

@pbeza pbeza linked an issue Dec 15, 2025 that may be closed by this pull request
Free storage used by the ProposedUpdatesEntriesV2 key in ProposedUpdates migrations #1664
Closed
@pbeza
Copy link
Contributor Author

pbeza commented Dec 15, 2025 •
edited
Loading

Ready for review! 🫡

The PR appears to correctly clean up the old storage entries, which you can verify by running this test:

mpc/crates/contract/src/v3_0_2_state.rs

Lines 287 to 329 in 41e7206

#[test]
fn test_migration_removes_v2_entries_from_storage() {
// given: Create old ProposedUpdates with entries in V2 storage
let mut old = create_old_proposed_updates(0);
let id1 = UpdateId(1);
let id2 = UpdateId(2);
old.entries.insert(
id1,
UpdateEntry {
update: Update::Contract(vec![1, 2, 3]),
votes: HashSet::new(),
bytes_used: 100,
},
);
old.entries.insert(
id2,
UpdateEntry {
update: Update::Contract(vec![4, 5, 6]),
votes: HashSet::new(),
bytes_used: 200,
},
);
// Verify V2 storage has entries before migration
assert_eq!(old.entries.len(), 2);
// when: Perform migration (which should remove V2 entries)
let _new: crate::update::ProposedUpdates = old.into();
// then: Create a new IterableMap with V2 storage key and verify it's empty
let v2_entries_after_migration: IterableMap<UpdateId, UpdateEntry> =
IterableMap::new(StorageKey::ProposedUpdatesEntriesV2);
// This proves .remove() deleted from storage, not just memory
assert_eq!(
v2_entries_after_migration.len(),
0,
"V2 storage should be empty after migration"
);
assert!(v2_entries_after_migration.get(&id1).is_none());
assert!(v2_entries_after_migration.get(&id2).is_none());
}

If you revert the other code changes from this PR (i.e., everything except the tests themselves), the test fails at line 327 (!):

mpc/crates/contract/src/v3_0_2_state.rs

Lines 321 to 328 in 41e7206

// This proves .remove() deleted from storage, not just memory
assert_eq!(
v2_entries_after_migration.len(),
0,
"V2 storage should be empty after migration"
);
assert!(v2_entries_after_migration.get(&id1).is_none());
assert!(v2_entries_after_migration.get(&id2).is_none());

This is surprising to me, because the check just above (assert_eq(...)) should fail first if the assertion on line 327 is going to fail. The number of keys is reported as 0 and the assertion on line 322 does not fail, yet we can still access 2 entries on lines 327 and 328. I’m not sure how to explain this — any ideas?

@pbeza pbeza marked this pull request as ready for review December 15, 2025 15:09
Copilot AI reviewed Dec 15, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

kevindeforth
kevindeforth approved these changes Dec 15, 2025
View reviewed changes
Copy link
Contributor

@kevindeforth kevindeforth left a comment

Choose a reason for hiding this comment

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

Thank you!

gilcu3
gilcu3 approved these changes Dec 16, 2025
View reviewed changes
Copy link
Contributor

@gilcu3 gilcu3 left a comment

Choose a reason for hiding this comment

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

LGTM!

left a question

@pbeza pbeza added this pull request to the merge queue Dec 16, 2025
Merged via the queue into main with commit 0faab75 Dec 16, 2025
14 checks passed
@pbeza pbeza deleted the 1664-free-storage-used-by-the-proposedupdatesentriesv2-key-in-proposedupdates-migrations branch December 16, 2025 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gilcu3 gilcu3 gilcu3 approved these changes

Copilot code review Copilot Copilot left review comments

@kevindeforth kevindeforth kevindeforth approved these changes

@netrome netrome Awaiting requested review from netrome

@DSharifi DSharifi Awaiting requested review from DSharifi

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Free storage used by the ProposedUpdatesEntriesV2 key in ProposedUpdates migrations

4 participants

@pbeza @gilcu3 @kevindeforth