Prototype SV governance voter flow#6
Conversation
Daml Review FeedbackI ran a fresh Daml review pass over the consolidated governance-voter prototype. The code builds and the Daml tests pass, but there are several points worth making explicit for reviewers. Key Findings
Overall AssessmentNo blocker was found for using this as a review prototype. The main review question is whether cross-channel overwrites are the desired Phase 1 semantics, or whether operator-cast votes should have precedence over governance-voter-cast votes. |
Follow-up Professor review after vote-slot fixesI reran Professor review after commit What Professor confirmed
Remaining Professor cautions / follow-up candidates
Local validation
|
agentdamo
left a comment
There was a problem hiding this comment.
Automated Review
PR: #6 — Prototype SV governance voter flow
Summary
Adds a Phase 1 governance-voter model to Splice DSO governance. An SV can declare a SvGovernanceVoter binding designating a separate party to cast votes on its behalf for an explicit allowlist of governance actions. Vote records are extended with castBy/castByRole for attribution. VoteRequest.votes is migrated from Map Text Vote to Map Party Vote.
Review
1. Correctness
DsoRules_CastVotesilently normalizes caller-suppliedcastBy/castByRole: The recorded vote always setscastBy = vote.svandcastByRole = VCR_Operatorregardless of what was passed. The controller isvote.svso attribution is correct, but this normalization is invisible to callers. Consider either arequireasserting the values match, or a comment explaining the intentional overwrite.executeAllDefinitiveVotesfix looks correct: Switching frommap (.name) $ Map.valuestoMap.keysaligns properly with the map-key migration. Good catch.Map Text Vote→Map Party Voteis a breaking data model change: Any existing in-flightVoteRequestcontracts on a live network would need a migration path. For a prototype this is expected, but should be flagged explicitly in the CIP/upstream PR so reviewers understand the upgrade surface.
2. Security
- Unilateral binding:
SvGovernanceVoterhassignatory svonly — the governance voter is an observer with no consent step. An SV can secretly designate any party. The docs flag this and propose a Propose-Accept shape as a follow-up, which is the right call. Worth flagging for CIP review: designating a governance voter without their consent could be surprising. - No contract key / one-active-binding not enforced at template level: An SV can create multiple
SvGovernanceVotercontracts simultaneously. Only the explicitly submittedbindingCidis validated, so this doesn't compromise vote integrity — but it creates confusion about which binding is canonical. AfetchByKeyor explicit de-dup check in the UI/workflow layer should be documented as a requirement. - Operator vote precedence is correct: The guard
pastVote.castByRole == VCR_Operator → failenforces that governance-voter casts cannot overwrite operator casts. Governance-voter-over-governance-voter overwrites are allowed, which matches the stated intent. - Action allowlist is correctly defensive:
_ -> Falsedefault ensures new constructors are rejected until deliberately reviewed.SRARC_OffboardSvinclusion is appropriately called out for CIP scrutiny. DsoRules_CastGovernanceVoteredundant but safe normalization: TherecordedVoteblock reassignscastBy = binding.governanceVoterandcastByRole = VCR_GovernanceVotereven though both were already validated by earlierrequires. This is good defense-in-depth — stored data always reflects the binding, not caller-supplied fields.
3. Clarity
DsoRules_CastVotenormalization: As noted above, the silent overwrite ofcastBy/castByRolein the operator path needs a comment. Otherwise a future maintainer may think the caller's values are honored.VoteCastRolederiving style:deriving(Eq, Show)is missing a space (deriving (Eq, Show)) — minor, but inconsistent with the surrounding Daml style.DsoRules_CastGovernanceVoteisnonconsuming(it archives and recreates the request manually). The pattern diverges fromDsoRules_CastVotewhich usesfetchAndArchive. A short comment explaining whyfetchChecked+ manual archive is used here (validation must occur before archiving) would help.
4. Edge Cases
- Multiple active bindings: Addressed above. No double-vote risk, but the workflow-layer invariant assumption should be documented more explicitly (current doc says "treated as an invariant to validate" but doesn't specify where/how).
- Governance voter == another active SV: Nothing prevents
sv1's governance voter from beingsv2's party.sv2could then cast governance-voter votes forsv1while also casting operator votes for itself. No double-counting occurs (different vote slots), but this could be confusing operationally. Worth a note in the docs or a comment. - Cooldown on first governance-voter cast: Correctly unenforced when no prior vote exists (the
Map.lookupreturnsNone). This is the right behavior.
5. Excellence
- Tests are comprehensive: binding lifecycle, casting/overwrite semantics, action taxonomy, and vote-slot tally are all covered. The use of
submitMustFailthroughout is idiomatic and thorough. - The doc page (
sv_governance_voter.rst) is clear and well-written. The explicit "prototype scope" framing is good. - Including the compiled DAR (
splice-dso-governance-0.1.25.dar) and updated lock file is appropriate for a prototype review.
DAML Professor
DAML Professor unavailable, skipping.
Automated review by Damo. PR is draft — comment only, no approval.
agentdamo
left a comment
There was a problem hiding this comment.
Approving at author's request (draft used to prevent accidental merge, not to indicate incompleteness). Findings in the prior comment are all suggestions — nothing blocking. LGTM for prototype/CIP review purposes.
Define the initial Phase 1 allowlist for governance-voter eligible actions and cover the proposed taxonomy plus represented-SV vote-slot semantics in Daml tests. Signed-off-by: Eric Mann <eric@avrofi.com>
Add explicit vote-cast role metadata so the vote record can identify the represented SV and operator signing path without changing tally semantics. Signed-off-by: Eric Mann <eric@avrofi.com>
Introduce the SV-declared governance-voter binding lifecycle without a contract key so the Phase 1 authority model can be reviewed independently of submit-path mechanics. Signed-off-by: Eric Mann <eric@avrofi.com>
Use the SV governance-voter binding to authorize alternate signing while recording the vote in the represented SV's existing vote slot. Signed-off-by: Eric Mann <eric@avrofi.com>
Keep the consolidated prototype to a single governance package version bump so intermediate stack-only DAR versions do not leak into the final review branch. Signed-off-by: Eric Mann <eric@avrofi.com>
Use represented SV parties as vote map keys and prevent governance-voter submissions from replacing prior operator votes, making the shared vote-slot semantics explicit for review. Signed-off-by: Eric Mann <eric@avrofi.com>
ef18735 to
e764089
Compare
samsondav
left a comment
There was a problem hiding this comment.
Reviewed against canton-network#5533 (the upstream prototype). The diff confirms this branch is earlier — a few of the larger gaps below have already evolved in the upstream version, so treat this as a prototype-review thread rather than a merge gate for this fork.
Three findings are load-bearing for both this branch and the upstream prototype:
controller vote.castByonDsoRules_CastGovernanceVoteis the choice-parameter-as-controller footgun. It is functionally correct today only because of five sequentialrequirechecks; engine-level authorization is degraded to handwritten runtime assertions.SvGovernanceVoterhas no on-ledger single-binding enforcement.RotateGovernanceVoterandClearGovernanceVoterare consuming on the source cid, but a barecreate SvGovernanceVoterby the SV signatory bypasses them. Two governance voters can simultaneously write to the samevotes[binding.sv]slot (subject to the operator-precedence carve-out in this branch). No test covers the failure mode. Daml 3.x droppedkey/maintainer, so the standard fix is unavailable — needs an off-ledger ACS check encoded via aDsoRules-mediated registration choice, or agovernanceVoterBindingCidfield inSvInfothat the cast choice gates against.- Schema break in
VoteRequest.votes(Map Text → Map Party) andVote(two new mandatory fields). LF 2.1 upgradability does not handle this in-place. Package-version bump is present but the PR description needs an explicit migration story (close all open requests before upgrade, or a one-shot reshape choice).
The rest of the inline comments are medium / low. Several PR-6-specific shapes (empty ClearGovernanceVoter body, lack of RequestGovernanceVote, operator-precedence carve-out, operator path with no eligibility check) appear already addressed in canton-network#5533 — flagging here mostly as breadcrumbs in case any of them slipped during the cherry-pick back.
Also: SRARC_OffboardSv in the allowlist is the highest-impact policy decision in the PR. Already flagged in PR description for upstream review — keep it loud in the CIP discussion.
Summary
This PR is a prototype for maintainer and CIP review, not a final upstream design. It makes the proposed Phase 1 SV governance-voter model concrete for discussion under Canton Development Fund PR canton-network#223, especially Milestone 1: Governance-Voting Identity and CIP.
Phase 1 preserves the existing one-vote-per-SV semantics. A governance voter is modeled as an alternate signer for a represented SV's vote, not as a new voting unit or source of additional vote weight.
Changes included:
isGovernanceVoterAction, with newActionRequiringConfirmationconstructors rejected by default until explicitly reviewed.VotewithcastByandcastByRoleso vote records distinguish the represented SV from the party/authority path that signed the vote.SvGovernanceVoter, an SV-declared binding template without a contract key; downstream paths fetch it by contract ID and validate the one-active-binding assumption at the workflow level.DsoRules_CastGovernanceVote, which validates the binding, represented SV, governance-voter signer, signer role, and action allowlist before writing the vote into the represented SV's existing vote slot.Design IDs covered: GV-001, GV-002, GV-003, GV-004, GV-005, GV-006.
Notes For Reviewers
SRARC_OffboardSvis included in the proposed Phase 1 allowlist so reviewers can evaluate it explicitly; this should be validated through maintainer/CIP review because it is a high-impact membership action.governanceVoter == svis allowed for bootstrap/self-voting, whilegovernanceVoter == dsois rejected.Test Plan
direnv exec . sbt "splice-dso-governance-test-daml/damlTest"direnv exec . sbt damlDarsLockFileUpdate