Skip to content

fix: code hashes can now be be voted for in all code protocol states#1620

Merged
DSharifi merged 9 commits intomainfrom
1589-bug-vote_code_hash-expects-contract-to-be-in-running-state
Dec 9, 2025
Merged

fix: code hashes can now be be voted for in all code protocol states#1620
DSharifi merged 9 commits intomainfrom
1589-bug-vote_code_hash-expects-contract-to-be-in-running-state

Conversation

@DSharifi
Copy link
Contributor

@DSharifi DSharifi commented Dec 8, 2025

closes #1589

@DSharifi DSharifi linked an issue Dec 8, 2025 that may be closed by this pull request
@DSharifi DSharifi marked this pull request as ready for review December 8, 2025 10:33
pbeza
pbeza previously approved these changes Dec 8, 2025
Copy link
Contributor

@pbeza pbeza left a comment

Choose a reason for hiding this comment

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

Left only a few optional nits. ✅

pbeza
pbeza previously approved these changes Dec 8, 2025
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.

Nice. So, right now, the big question is what to do with the TestSetup and TestSetupBuilder structs.

I like the proposed ergonomics and the user interface. .with_participant_count(participants) feels very nice.

I don't like that it relies so heavily on implemented functions and that it duplicates existing logic. For that, I think it would be good to leverage existing logic in our test_utils files.

The risk right now is that we steer towards maintaining two testing libraries for the same thing.

@kevindeforth
Copy link
Contributor

I would suggest to take the interface you propose, as it's very nice, but implement it in the existing test_utils.rs files. This would achieve three things:

  • it makes your nice interface accessible to other tests in the contract folder
  • it unifies our test setup logic, so we only maintain one, not two
  • it means when we generate a contract state, we generate it without relying on any voting paths, but can just insert the values in the struct.

@DSharifi DSharifi requested a review from kevindeforth December 9, 2025 09:28
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.

Thanks for the changes.

Copy link
Collaborator

@netrome netrome left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for adding the test case! 🙏

@DSharifi DSharifi added this pull request to the merge queue Dec 9, 2025
Merged via the queue into main with commit e2453f6 Dec 9, 2025
13 checks passed
@DSharifi DSharifi deleted the 1589-bug-vote_code_hash-expects-contract-to-be-in-running-state branch December 9, 2025 17:02
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.

bug: vote_code_hash expects contract to be in running state

4 participants