feat(governance): show how many approvers still need to approve - #788
Open
pahor167 wants to merge 5 commits into
Open
feat(governance): show how many approvers still need to approve#788pahor167 wants to merge 5 commits into
pahor167 wants to merge 5 commits into
Conversation
`governance:show` and `multisig:show` reported who can approve but not how many signatures are still missing. The proposal view's `completion` field counted approver multisig owners, which is not the threshold that approves a proposal, so a fully approved proposal could read as "3 / 12". - GovernanceWrapper.getApprovalStatus returns `required` (the approver multisig threshold) and `remaining`; `completion` keeps its meaning - governance:show --proposalID prints the new fields plus a note with the outstanding confirmation count - governance:show --hotfix gained an approvals section covering both the approver and the security council, falling back to a single signature when the address is not a multisig - multisig:show reports confirmations, confirmationsRequired and confirmationsRemaining per transaction, using the internal threshold for transactions the multisig sends to itself, as MultiSig.isConfirmed does
🦋 Changeset detectedLatest commit: 6d1105e The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves how governance- and multisig-related approval progress is surfaced across the SDK and celocli, by reporting the true multisig threshold (required()), how many confirmations have been collected, and how many remain—avoiding the misleading “confirmations / owners” interpretation when the threshold is smaller than the owner set.
Changes:
- SDK: Exported
ApprovalStatusand extendedGovernanceWrapper.getApprovalStatusto returnrequiredandremainingwhile preservingcompletionfor backwards compatibility. - CLI: Enhanced
governance:show(proposal + hotfix paths) andmultisig:showto display confirmations-required/remaining and user-facing note lines for pending approvals. - Refactor + tests: Centralized confirmation-progress math in CLI utilities and added/updated unit + integration coverage and snapshots.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/sdk/contractkit/src/wrappers/Governance.ts | Export ApprovalStatus; include required + remaining in getApprovalStatus derived from multisig threshold. |
| packages/sdk/contractkit/src/wrappers/Governance.test.ts | Add coverage for new required/remaining behavior in getApprovalStatus. |
| packages/cli/src/utils/multisig-utils.ts | Add getConfirmationProgress helper (regular vs internal threshold, executed handling). |
| packages/cli/src/utils/multisig-utils.test.ts | Unit tests for getConfirmationProgress edge cases and threshold selection. |
| packages/cli/src/utils/governance.ts | Add helpers to compute and return hotfix/multisig approval progress (approver + security council). |
| packages/cli/src/commands/multisig/show.ts | Display per-tx confirmation progress and handle missing tx ids by omitting progress fields. |
| packages/cli/src/commands/multisig/show.test.ts | Integration test proving internal-threshold behavior in multisig:show. |
| packages/cli/src/commands/governance/show.ts | Print approvals section and “remaining confirmations” notes for proposals and hotfixes. |
| packages/cli/src/commands/governance/show.test.ts | Snapshot update + new hotfix test covering pending → approved output. |
| .changeset/remaining-approvals.md | Changeset documenting the CLI/SDK feature additions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
size-limit report 📦
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #788 +/- ##
==========================================
+ Coverage 64.52% 67.27% +2.75%
==========================================
Files 33 170 +137
Lines 919 10042 +9123
Branches 114 1848 +1734
==========================================
+ Hits 593 6756 +6163
- Misses 313 3198 +2885
- Partials 13 88 +75
🚀 New features to boost your workflow:
|
Mc01
reviewed
Aug 4, 2026
added 2 commits
August 5, 2026 10:20
getConfirmations issued one confirmations() read per owner; the contract already exposes getConfirmations(txId) returning the full list in one call. getTransactionDataByContent fetched the multisig's entire transaction history in a single Promise.all (268 parallel requests on the mainnet approver multisig), which is enough to get rate-limited by public RPCs. It now scans newest-first in small batches and stops at the first match; the wanted transaction is almost always recent. When several transactions share the same content, the most recent one is now returned instead of the oldest.
- Classify the hotfix approver with getCode() instead of probing required() and catching the revert. An EOA, a Celo MultiSig, a Gnosis Safe and an unknown contract are now distinct kinds; for a Safe the real getThreshold() is reported instead of pretending a single signature suffices. - Fetch the multisig threshold once per approval path and pass it down, instead of reading required() twice. - For an approved hotfix, resolve the confirmation list from the executed multisig transaction instead of returning an empty placeholder. - Unify ConfirmationProgress on required/remaining to match ApprovalStatus; multisig:show keeps its prefixed display keys, which would otherwise be ambiguous next to the contract-wide required-confirmations lines. - Use synthetic addresses in the multisig-utils unit tests; the previous constants were real mainnet accounts.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (1)
packages/cli/src/utils/multisig-utils.ts:27
getConfirmationProgresscorrectly accounts for the multisig’sinternalRequiredthreshold (when the tx destination is the multisig itself), butviewConfirmationStatus(used bymultisig:approve/multisig:transfer) still reads onlyrequired(). As a result, for self-targeted transactions (e.g. add/remove owner) the CLI can misreport how many confirmations are needed and whether the next approval will execute the tx, even thoughmultisig:shownow reports the internal-threshold numbers.
export async function getConfirmationProgress(
readMultisig: Pick<ConfirmationGetters, 'getConfirmations'>,
txIndex: bigint,
transaction: { destination: Address; executed: boolean },
thresholds: { required: bigint; internalRequired: bigint },
multisigAddress: Address
Cover getTransactionDataByContent against a stubbed contract: first-batch hit, multi-batch continuation, newest-of-duplicates, exhaustive miss, and value matching, plus the single-read getConfirmations. Also lifts the diff coverage of the batched-scan change above the patch threshold, which the anvil-based Governance tests alone misattribute under the es6 async transform.
Exercise submitOrConfirmTransaction (confirm-existing and submit-new paths), getTransaction with confirmations, getTransactions, the owner and threshold getters and the write helpers against the stubbed contract, bringing the wrapper to near-full line coverage.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (2)
packages/cli/src/commands/multisig/show.ts:58
multisig:showcurrently treatsdestination === zeroAddressas “transaction does not exist”. That can incorrectly hide confirmation progress for a real transaction that intentionally targets the zero address. A safer existence check is to comparetxIdagainsttxCount(which you already fetched).
if (destination === zeroAddress) {
// transaction does not exist, there is nothing to confirm
return {}
}
.changeset/remaining-approvals.md:11
- The release note for
governance:show --hotfixsays it reports progress towards thresholds, but it doesn’t mention the new Safe-specific behavior (readinggetThreshold()whenrequired()is unavailable). Adding that detail here will match what the code actually does and set expectations that partial Safe progress isn’t visible.
- `governance:show --hotfix` gained an `approvals` section with the approver and security
council progress towards their respective thresholds.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Nothing in celocli told an approver how many more signatures a pending approval still needs.
Worse,
governance:show --proposalIDreportedcompletionasconfirmations / owners. Owners is not the threshold that approves a proposal —MultiSig.required()is. On mainnet the approver multisig has 12 owners and a threshold of 3, so a fully approved proposal reads ascompletion: 3 / 12, which looks 25% done.This surfaces the real numbers everywhere an approval is pending: confirmations collected, the required threshold, and how many approvers still have to sign.
GovernanceWrapper.getApprovalStatusreturnsrequiredandremaining.completionkeeps its existing meaning so SDK consumers are unaffected;ApprovalStatusis now exported.governance:show --proposalIDshows the new fields in theapprovalsmap plus a note line.governance:show --hotfixgained anapprovalssection for both approval paths (approver and security council), each with address, confirmations, required and remaining, plus a note per path.multisig:showreportsconfirmations,confirmationsRequiredandconfirmationsRemainingper displayed transaction. It uses the internal threshold for transactions the multisig sends to itself, matchingMultiSig.isConfirmed, and skips the fields for non-existent transaction ids.No new flags, so no docs regeneration.
Other changes
Confirmation-progress math lives in
getConfirmationProgress(utils/multisig-utils.ts) andgetMultiSigApprovalProgress/getHotfixApprovalProgress(utils/governance.ts) rather than inline in the commands.A security council or approver that is not a Celo MultiSig (mainnet's security council is a Gnosis Safe) has no
required(), so it falls back to reporting a single outstanding signature. Reading a Safe's owngetThreshold()is left for a follow-up.Tested
packages/sdk/contractkit/src/wrappers/Governance.test.ts— new#getApprovalStatuscase:remaining === requiredbefore approval,0with the confirming address after. Suite green (12/12).packages/cli/src/utils/multisig-utils.test.ts— new unit tests forgetConfirmationProgress: regular vs internal threshold, executed transactions, and no negative remainder when a threshold was lowered.packages/cli/src/commands/multisig/show.test.ts— new integration test on a dedicated 3-owner multisig (required3,internalRequired2) proving the internal-threshold branch: proposing leavesconfirmationsRemaining: 1, a second confirmation executes it and drops it to 0. Existing snapshot updated.packages/cli/src/commands/governance/show.test.ts— proposal snapshot updated; new hotfix test walking pending -> approved.@celo/celoclisuite: 102/103 suites, 392 tests. The one failure issrc/utils/helpers.test.tsfailing to bind an anvil port under parallel workers (Address already in use); it passes in isolation and is untouched by this change.Verified against a mainnet fork by impersonating three approver multisig signatories and approving live proposal 303:
with the note line tracking it:
Note: 2 more approver confirmation(s) needed to approve this proposal (1/3)->(2/3)-> gone once approved.How to QA
Read-only against mainnet:
For the proposal view, a proposal in Referendum or Execution with at least one confirmation is needed; at the time of writing mainnet has none, so use a fork:
Note that forno rate-limits the multisig transaction scan and prunes historical state, so forking needs an archive endpoint.
Related issues
PR-Codex overview
This PR focuses on optimizing the MultiSigWrapper's read operations to reduce RPC load and enhance governance approval tracking within the Celo ecosystem.
Detailed summary
getConfirmationsto use a contract view for efficiency.getTransactionDataByContentto scan transactions in batches, returning the most recent match.GovernanceWrapperto return new fields for required and remaining approvals.MultiSigWrapperto optimize transaction fetching and confirmations handling.