fix: include oz_governor proposer in transitive sender resolution#62
Open
nvtaveras wants to merge 2 commits into
Open
fix: include oz_governor proposer in transitive sender resolution#62nvtaveras wants to merge 2 commits into
nvtaveras wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughUpdated Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
When an oz_governor sender references a proposer account (e.g. "dev-pk"), that account must be included in SENDER_CONFIGS so the Solidity-side registry can look it up during OZGovernorSender.initialize(). Previously, only safe senders had their transitive dependencies (signers) resolved. The oz_governor proposer was missing, causing a revert with InvalidOZGovernorConfig when the Solidity code tried to validate the proposer sender type.
nvtaveras
force-pushed
the
fix/missingGovernorSender
branch
from
017b475 to
124c629
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/config/senders.go (1)
91-92:⚠️ Potential issue | 🟡 MinorPre-existing bug: Error message mentions "ledger" but validates Trezor.
The error message on line 92 says "can not use ledger" but the condition checks
signersHWConfig.UseTrezor && executionHWConfig.UseTrezor. This is a copy-paste error from line 88.Not introduced by this PR, but worth fixing while you're here:
Proposed fix
if signersHWConfig.UseTrezor && executionHWConfig.UseTrezor { - return nil, fmt.Errorf("can not use ledger in both main sender and safe signer, configure `@custom`:senders") + return nil, fmt.Errorf("can not use trezor in both main sender and safe signer, configure `@custom`:senders") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/senders.go` around lines 91 - 92, The error message is incorrect for the Trezor check: when signersHWConfig.UseTrezor && executionHWConfig.UseTrezor is true, update the fmt.Errorf message to mention Trezor (not ledger) so it matches the condition; locate the check in the function that uses signersHWConfig.UseTrezor and executionHWConfig.UseTrezor and replace the string passed to fmt.Errorf with a descriptive message like "can not use trezor in both main sender and safe signer, configure `@custom`:senders".
🧹 Nitpick comments (3)
internal/config/senders.go (3)
85-85: Consider renamingsignersHWConfigfor clarity.Now that this variable covers both safe signers and governor proposers, a name like
transitiveHWConfigordependenciesHWConfigwould better reflect its purpose.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/senders.go` at line 85, Rename the variable signersHWConfig to a clearer name (e.g., transitiveHWConfig or dependenciesHWConfig) to reflect that it covers both safe signers and governor proposers; update the declaration that assigns m.getSendersHWConfig(transitiveSenders) and then rename every usage of signersHWConfig in this file (and any nearby functions referencing it) to the new identifier so references remain consistent (ensure getSendersHWConfig and transitiveSenders identifiers remain unchanged).
95-97: Potential duplicate entries when a sender is both declared and a transitive dependency.If a user declares
@custom:senders safe0, signer0wheresafe0.signer = "signer0", thensigner0appears in bothsendersandtransitiveSenders, resulting in duplicates inallSenders. This causes duplicate entries inSenderInitConfigs.Proposed fix to deduplicate
sort.Strings(transitiveSenders) sort.Strings(senders) - allSenders := append(slices.Clone(transitiveSenders), senders...) + // Deduplicate: transitive senders that are already declared don't need to be prepended + seen := make(map[string]bool, len(senders)) + for _, s := range senders { + seen[s] = true + } + allSenders := make([]string, 0, len(transitiveSenders)+len(senders)) + for _, s := range transitiveSenders { + if !seen[s] { + allSenders = append(allSenders, s) + } + } + allSenders = append(allSenders, senders...)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/senders.go` around lines 95 - 97, The concatenation of transitiveSenders and senders into allSenders can produce duplicates (e.g., when an entry appears in both transitiveSenders and senders), which leads to duplicate SenderInitConfigs; change the construction of allSenders (referencing transitiveSenders, senders, allSenders, and resulting SenderInitConfigs) to deduplicate entries—for example, iterate over slices.Clone(transitiveSenders) then append only those senders from senders that are not already present (use a temporary map[string]struct{} set to track seen names) so allSenders contains each sender exactly once before downstream use.
77-82: Guard against empty transitive references before appending.If a
safeoroz_governorsender is misconfigured with an emptySignerorProposer, this code appends an empty string totransitiveSenders. Later,buildSenderInitConfigswould fail with"sender '' not found in configuration", which is a confusing error message.Consider validating non-empty before appending:
Proposed fix
switch sender.Type { case "safe": - transitiveSenders = append(transitiveSenders, sender.Signer) + if sender.Signer != "" { + transitiveSenders = append(transitiveSenders, sender.Signer) + } case "oz_governor": - transitiveSenders = append(transitiveSenders, sender.Proposer) + if sender.Proposer != "" { + transitiveSenders = append(transitiveSenders, sender.Proposer) + } }This allows the existing validation in
buildSenderInitConfig(lines 252 and 341) to produce the appropriate error message when processing the parent sender.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/senders.go` around lines 77 - 82, The switch in senders.go is appending potentially-empty transitive references which leads to confusing errors later; update the switch handling in the function that builds transitiveSenders so that before appending sender.Signer (for "safe") or sender.Proposer (for "oz_governor") you check for non-empty (e.g., != "" or len(...)>0) and only append when non-empty, otherwise skip (or collect a clearer validation error earlier); ensure you touch the transitiveSenders variable and keep buildSenderInitConfigs unchanged so later validation on the parent sender produces the proper error message.
🤖 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 `@internal/config/senders.go`:
- Around line 91-92: The error message is incorrect for the Trezor check: when
signersHWConfig.UseTrezor && executionHWConfig.UseTrezor is true, update the
fmt.Errorf message to mention Trezor (not ledger) so it matches the condition;
locate the check in the function that uses signersHWConfig.UseTrezor and
executionHWConfig.UseTrezor and replace the string passed to fmt.Errorf with a
descriptive message like "can not use trezor in both main sender and safe
signer, configure `@custom`:senders".
---
Nitpick comments:
In `@internal/config/senders.go`:
- Line 85: Rename the variable signersHWConfig to a clearer name (e.g.,
transitiveHWConfig or dependenciesHWConfig) to reflect that it covers both safe
signers and governor proposers; update the declaration that assigns
m.getSendersHWConfig(transitiveSenders) and then rename every usage of
signersHWConfig in this file (and any nearby functions referencing it) to the
new identifier so references remain consistent (ensure getSendersHWConfig and
transitiveSenders identifiers remain unchanged).
- Around line 95-97: The concatenation of transitiveSenders and senders into
allSenders can produce duplicates (e.g., when an entry appears in both
transitiveSenders and senders), which leads to duplicate SenderInitConfigs;
change the construction of allSenders (referencing transitiveSenders, senders,
allSenders, and resulting SenderInitConfigs) to deduplicate entries—for example,
iterate over slices.Clone(transitiveSenders) then append only those senders from
senders that are not already present (use a temporary map[string]struct{} set to
track seen names) so allSenders contains each sender exactly once before
downstream use.
- Around line 77-82: The switch in senders.go is appending potentially-empty
transitive references which leads to confusing errors later; update the switch
handling in the function that builds transitiveSenders so that before appending
sender.Signer (for "safe") or sender.Proposer (for "oz_governor") you check for
non-empty (e.g., != "" or len(...)>0) and only append when non-empty, otherwise
skip (or collect a clearer validation error earlier); ensure you touch the
transitiveSenders variable and keep buildSenderInitConfigs unchanged so later
validation on the parent sender produces the proper error message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 29d3569c-c958-4d9a-b015-780333d83d61
📒 Files selected for processing (1)
internal/config/senders.go
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
1 participant
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.
Summary
oz_governorsender (e.g.@custom:senders deployer, governor), the governor'sproposeraccount (e.g.dev-pk) was not included in the encodedSENDER_CONFIGSpassed to Solidity. This causedOZGovernorSender.initialize()to revert withInvalidOZGovernorConfigbecause it couldn't find the proposer in the sender registry.BuildSenderScriptConfigresolved transitive sender dependencies forsafesenders (including theirsigner), but did not do the same foroz_governorsenders and theirproposerfield.oz_governorsenders by including theirproposer, using the same pattern already in place forsafesigners.Details
The error manifested as:
This is
InvalidOZGovernorConfig("governor")fromOZGovernorSender.sol:112.The flow:
@custom:senders deployer, governorfrom the script's natspecSenderInitConfigstructsSENDER_CONFIGSenv varSENDER_CONFIGSand registers each sender bykeccak256(name)in theSendersregistryOZGovernorSender.initialize()decodes the governor's config to findproposerName = "dev-pk", then callsSenders.get(keccak256("dev-pk"))to look up the proposerdev-pkwas never registered — onlydeployerwas (which uses the same underlying private key but is registered under a different name)senderType = 0x0, which fails the type validation → revertThe fix adds
oz_governorto the sameswitchblock that already handlessafesigners, sodev-pk(or whatever the proposer is) gets included as a transitive dependency.Summary by CodeRabbit