Conversation
📝 WalkthroughWalkthroughAdds persistence and clearing for enacted committee quorum. A new 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
database/plugin/metadata/mysql/committee_member.go (1)
90-150:⚠️ Potential issue | 🟡 MinorBoth
SetCommitteeQuorumandClearCommitteeQuorumupsert on the same unique key—document the at-most-one-mutation-per-slot assumption.The
added_slotcolumn does have aUNIQUEconstraint (verified in model definition), so the upsert works correctly and duplicates will not pile up. However, both methods share the same conflict key, so ifUpdateCommitteeandNoConfidenceenact at the same slot, the second call silently overwrites the first's marker. While current Conway rules prevent this (only one ratified proposal per conflicting class per epoch), a one-line comment noting "at most one committee-quorum mutation per slot" would guard against future atomic-enactment changes.The
Sign() <= 0nil-guard is correctly safe via short-circuit evaluation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@database/plugin/metadata/mysql/committee_member.go` around lines 90 - 150, Both ClearCommitteeQuorum and SetCommitteeQuorum perform an upsert on the same unique key (added_slot) so the second mutation at the same slot will overwrite the first; add a one-line comment near the upsert logic in ClearCommitteeQuorum and SetCommitteeQuorum (or above the methods) stating the assumption "at most one committee-quorum mutation per slot (unique on added_slot)" to document why this behavior is acceptable and to warn future maintainers (reference functions ClearCommitteeQuorum, SetCommitteeQuorum and the added_slot unique key).
🧹 Nitpick comments (2)
ledger/governance/enact_test.go (1)
201-240: Test relies on implicit zero-deposit short-circuit in refundProposalDeposit.The comment notes the zero-deposit path short-circuits the reward-account refund, but the test also doesn't configure a reward-address lookup or network state. If
refundProposalDepositever grows a non-zero-deposit code path that runs unconditionally (e.g. metric/log on return address decode), this test will start failing for unrelated reasons. Consider explicitly settingproposal.Deposit = 0to make the intent visible even if the field already defaults to zero.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ledger/governance/enact_test.go` around lines 201 - 240, The test relies on an implicit zero deposit when calling EnactProposal which makes the zero-deposit short-circuit in refundProposalDeposit fragile; explicitly set proposal.Deposit = 0 in TestEnactProposal_NoConfidence_ClearsCommitteeQuorum to document intent and prevent future failures if refundProposalDeposit adds non-short-circuit logic (update the models.GovernanceProposal instance before calling EnactProposal to set Deposit to 0, and keep the test’s existing assertions around EnactProposal and db.GetCommitteeQuorum).database/plugin/metadata/sqlite/committee_member_test.go (1)
267-308: Good coverage for the clear + rollback semantics.Both tests exercise the key invariants (clear hides prior quorum; rollback past the clear restores it). Consider adding one more case that calls
ClearCommitteeQuorumwhen no prior quorum exists (start-of-chain) to lock in the "never enacted + cleared marker ⇒ still nil" contract, but this is a nice-to-have.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@database/plugin/metadata/sqlite/committee_member_test.go` around lines 267 - 308, Add a test that verifies ClearCommitteeQuorum when no prior quorum exists preserves a nil result: create a new test (e.g., TestClearCommitteeQuorumWhenNoPrior) that uses setupTestStore, calls store.ClearCommitteeQuorum with some slot and nil tx (without any prior SetCommitteeQuorum), then asserts store.GetCommitteeQuorum(nil) returns nil, and finally verifies a subsequent SetCommitteeQuorum (e.g., at a later slot) still returns the newly set quorum; reference the existing helpers and methods SetCommitteeQuorum, ClearCommitteeQuorum, and GetCommitteeQuorum to implement this case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@database/plugin/metadata/mysql/committee_member.go`:
- Around line 90-150: Both ClearCommitteeQuorum and SetCommitteeQuorum perform
an upsert on the same unique key (added_slot) so the second mutation at the same
slot will overwrite the first; add a one-line comment near the upsert logic in
ClearCommitteeQuorum and SetCommitteeQuorum (or above the methods) stating the
assumption "at most one committee-quorum mutation per slot (unique on
added_slot)" to document why this behavior is acceptable and to warn future
maintainers (reference functions ClearCommitteeQuorum, SetCommitteeQuorum and
the added_slot unique key).
---
Nitpick comments:
In `@database/plugin/metadata/sqlite/committee_member_test.go`:
- Around line 267-308: Add a test that verifies ClearCommitteeQuorum when no
prior quorum exists preserves a nil result: create a new test (e.g.,
TestClearCommitteeQuorumWhenNoPrior) that uses setupTestStore, calls
store.ClearCommitteeQuorum with some slot and nil tx (without any prior
SetCommitteeQuorum), then asserts store.GetCommitteeQuorum(nil) returns nil, and
finally verifies a subsequent SetCommitteeQuorum (e.g., at a later slot) still
returns the newly set quorum; reference the existing helpers and methods
SetCommitteeQuorum, ClearCommitteeQuorum, and GetCommitteeQuorum to implement
this case.
In `@ledger/governance/enact_test.go`:
- Around line 201-240: The test relies on an implicit zero deposit when calling
EnactProposal which makes the zero-deposit short-circuit in
refundProposalDeposit fragile; explicitly set proposal.Deposit = 0 in
TestEnactProposal_NoConfidence_ClearsCommitteeQuorum to document intent and
prevent future failures if refundProposalDeposit adds non-short-circuit logic
(update the models.GovernanceProposal instance before calling EnactProposal to
set Deposit to 0, and keep the test’s existing assertions around EnactProposal
and db.GetCommitteeQuorum).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 37baf4fe-4157-4941-83e4-e80f99aae0f3
📒 Files selected for processing (9)
database/committee.godatabase/plugin/metadata/mysql/committee_member.godatabase/plugin/metadata/postgres/committee_member.godatabase/plugin/metadata/sqlite/committee_member.godatabase/plugin/metadata/sqlite/committee_member_test.godatabase/plugin/metadata/store.goledger/governance/enact.goledger/governance/enact_test.goledger/governance/ratify_test.go
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Signed-off-by: Chris Gianelloni <wolf31o2@blinklabs.io>
wolf31o2
force-pushed
the
feat/committee-quorum-persistence
branch
from
cd7140a to
cb2409f
Compare
1 participant
Closes #1967
Summary by CodeRabbit
New Features
Tests
Summary by cubic
Persist committee quorum updates and add a clear state. No-confidence clears the quorum; ratification falls back to Conway genesis until a new quorum is set.
ClearCommitteeQuorum(slot, txn)toMetadataStorewithmysql,postgres, andsqliteimplementations, exposed viadatabase;GetCommitteeQuorumreturns nil when cleared.UpdateCommitteepersists positive quorum;NoConfidencecallsClearCommitteeQuorum; ratification prefers the DB quorum over genesis and falls back to genesis after a clear. Upserts byadded_slotkeep mutations idempotent and limited to one per slot.Written for commit cb2409f. Summary will update on new commits.