🐛 fee config bug fix and target added to tx fee validations#156
🐛 fee config bug fix and target added to tx fee validations#156garry-sharp wants to merge 1 commit into
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRefactors fee config loading to parse chain_id via a new wrapper and adds a DryRun config option. Tightens proposeTransactions to require exactly one ERC‑20 transfer rule targeting the treasury (USDC), resolves the treasury, verifies token address, and preserves signing/payload generation. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ConfigLoader
participant ViperDecoder
participant FeeConfig
Caller->>ConfigLoader: WithFileConfig(path)
ConfigLoader->>ViperDecoder: Unmarshal into FeeConfigFileWrapper
ViperDecoder-->>ConfigLoader: {FeeConfig, ChainIdRaw}
ConfigLoader->>FeeConfig: Copy embedded FeeConfig
ConfigLoader->>FeeConfig: Set ChainId from ChainIdRaw
ConfigLoader-->>Caller: Config (including DryRun, ChainId)
sequenceDiagram
participant PolicyRunner
participant Proposer
participant TreasuryResolver
participant Signer
PolicyRunner->>Proposer: proposeTransactions(run with 1 rule)
Proposer->>Proposer: Validate resource == ethereum.erc20.transfer
Proposer->>Proposer: Ensure recipient constraint == VULTISIG_TREASURY (MAGIC_CONSTANT)
Proposer->>TreasuryResolver: Resolve ethereum.usdc treasury
TreasuryResolver-->>Proposer: Recipient address
Proposer->>Proposer: Verify target token address == USDC
Proposer->>Signer: Build transfer payload (amount = run.TotalAmount)
Signer-->>Proposer: Unsigned payload + signature, tx hash
Proposer-->>PolicyRunner: Return KeysignRequest(s)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-07-04T10:47:47.927ZApplied to files:
🔇 Additional comments (6)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes fee configuration bugs and adds dry run functionality while improving transaction fee validation. The changes restructure the fee policy validation logic to be more robust and add support for a dry run mode.
- Simplified rule validation from a loop to single rule validation with explicit error handling
- Added dry run configuration option with proper file configuration wrapper
- Enhanced chain ID validation to prevent zero values and improved configuration structure
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| plugin/fees/transaction.go | Refactored rule processing from loop to single rule validation, extracted token from rule target, and fixed indentation |
| plugin/fees/config.go | Added dry run functionality and restructured config with wrapper to properly handle chain ID initialization |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
plugin/fees/transaction.go (1)
245-254: Validation is evaluating the tx-hash instead of the raw tx bytesYou’re decoding base64(Message), but Message is the SHA-256 input (tx hash bytes), not the transaction. The recipe engine needs the actual transaction bytes. Use the hex-encoded unsigned tx from PluginKeysignRequest.Transaction.
Apply this diff:
- txBytes, err := base64.StdEncoding.DecodeString(keysignMessage.Message) - if err != nil { - return fmt.Errorf("failed to decode transaction: %w", err) - } + txBytes, err := hexutil.Decode(tx.Transaction) + if err != nil { + return fmt.Errorf("failed to decode transaction hex: %w", err) + }
🧹 Nitpick comments (2)
plugin/fees/transaction.go (2)
91-93: Tighten error message for non-treasury recipientMake the error actionable by reporting the actual magic constant found.
- if magicConstantRecipientValue != rtypes.MagicConstant_VULTISIG_TREASURY { - return nil, fmt.Errorf("recipient constraint is not a treasury magic constant") - } + if magicConstantRecipientValue != rtypes.MagicConstant_VULTISIG_TREASURY { + return nil, fmt.Errorf("recipient constraint must be VULTISIG_TREASURY; got %v", magicConstantRecipientValue) + }
68-70: Comment says “we’ll loop through them all” but code enforces exactly oneMinor doc mismatch that could confuse future readers.
- // This should only return one rule, but in case there are more/fewer rules, we'll loop through them all and error if it's the case. + // This should contain exactly one rule; error if the count differs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
plugin/fees/config.go(6 hunks)plugin/fees/transaction.go(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: garry-sharp
PR: vultisig/plugin#117
File: plugin/fees/policy.go:46-47
Timestamp: 2025-07-04T10:47:47.927Z
Learning: For the fee plugin in plugin/fees/policy.go, the resource validation and recipe specification are intentionally configured to accept only USDC transfers ("ethereum.usdc.transfer"), not general ERC20 transfers ("ethereum.erc20.transfer"), as fees are only collected in USDC.
Learnt from: garry-sharp
PR: vultisig/plugin#146
File: plugin/fees/transaction.go:158-160
Timestamp: 2025-07-31T15:43:42.631Z
Learning: In the vultisig/plugin fees plugin, the public ProposeTransactions method intentionally returns "not implemented" because the fees plugin v2 uses a run-based architecture instead of the standard plugin template pattern. The actual transaction proposal logic is implemented in the private proposeTransactions method that accepts a types.FeeRun parameter, allowing fees to be batched and tracked through fee runs rather than individual policy-based transactions.
📚 Learning: 2025-07-04T10:47:47.927Z
Learnt from: garry-sharp
PR: vultisig/plugin#117
File: plugin/fees/policy.go:46-47
Timestamp: 2025-07-04T10:47:47.927Z
Learning: For the fee plugin in plugin/fees/policy.go, the resource validation and recipe specification are intentionally configured to accept only USDC transfers ("ethereum.usdc.transfer"), not general ERC20 transfers ("ethereum.erc20.transfer"), as fees are only collected in USDC.
Applied to files:
plugin/fees/transaction.goplugin/fees/config.go
📚 Learning: 2025-07-31T15:43:42.631Z
Learnt from: garry-sharp
PR: vultisig/plugin#146
File: plugin/fees/transaction.go:158-160
Timestamp: 2025-07-31T15:43:42.631Z
Learning: In the vultisig/plugin fees plugin, the public ProposeTransactions method intentionally returns "not implemented" because the fees plugin v2 uses a run-based architecture instead of the standard plugin template pattern. The actual transaction proposal logic is implemented in the private proposeTransactions method that accepts a types.FeeRun parameter, allowing fees to be batched and tracked through fee runs rather than individual policy-based transactions.
Applied to files:
plugin/fees/transaction.go
📚 Learning: 2025-07-24T19:34:47.441Z
Learnt from: webpiratt
PR: vultisig/plugin#141
File: plugin/payroll/transaction.go:208-267
Timestamp: 2025-07-24T19:34:47.441Z
Learning: In the vultisig/plugin codebase, duplicate transaction prevention for the payroll plugin has been moved to the verifier side, so the local `IsAlreadyProposed` check in plugin/payroll/transaction.go is no longer needed. The validation flow has changed and is now handled by the verifier rather than within the plugin itself.
Applied to files:
plugin/fees/transaction.go
🧬 Code Graph Analysis (1)
plugin/fees/transaction.go (1)
internal/types/keysign.go (1)
PluginKeysignRequest(7-12)
🔇 Additional comments (6)
plugin/fees/transaction.go (3)
95-99: LGTM: Treasury resolution for (ethereum, usdc)Using the default resolver with the treasury magic constant matches the intended flow.
101-106: LGTM: Target address check against configured USDCVerifying the target address equals the configured USDC address closes a class of mis-targeted transfers.
78-89: Constraint decode bug: using FixedValue for a MAGIC_CONSTANTFor a MAGIC_CONSTANT constraint, use GetMagicConstant() instead of GetFixedValue() + strconv.ParseInt. The current logic will misparse and likely break when the oneof is set to MagicConstant.
Apply this diff:
- for _, constraint := range rule.ParameterConstraints { - if constraint.ParameterName == "recipient" { - if constraint.Constraint.Type != rtypes.ConstraintType_CONSTRAINT_TYPE_MAGIC_CONSTANT { - return nil, fmt.Errorf("recipient constraint is not a magic constant") - } - iv, err := strconv.ParseInt(constraint.Constraint.GetFixedValue(), 10, 64) - if err != nil { - return nil, fmt.Errorf("failed to parse fixed value: %v", err) - } - magicConstantRecipientValue = rtypes.MagicConstant(iv) - } - } + for _, constraint := range rule.ParameterConstraints { + if constraint.ParameterName == "recipient" { + if constraint.Constraint.Type != rtypes.ConstraintType_CONSTRAINT_TYPE_MAGIC_CONSTANT { + return nil, fmt.Errorf("recipient constraint is not a magic constant") + } + mc := constraint.Constraint.GetMagicConstant() + if mc == rtypes.MagicConstant_UNSPECIFIED { + return nil, fmt.Errorf("recipient magic constant is unspecified") + } + magicConstantRecipientValue = mc + } + }Note: After this change, the strconv import becomes unused; please remove it.
⛔ Skipped due to learnings
Learnt from: garry-sharp PR: vultisig/plugin#143 File: plugin/fees/transaction.go:72-80 Timestamp: 2025-07-28T23:15:39.445Z Learning: In the vultisig/plugin codebase, magic constants (CONSTRAINT_TYPE_MAGIC_CONSTANT) use GetFixedValue() to retrieve their values, not a separate GetMagicConstantValue() method. The constraint type validation ensures it's the correct type, but value retrieval uses the unified GetFixedValue() method across all constraint types.plugin/fees/config.go (3)
39-42: Wrapper type for chain_id parsing is a good additionEmbedding FeeConfig and separating ChainIdRaw simplifies unmarshalling without changing the public FeeConfig shape.
109-115: WithDryRun option looks goodClear, minimal surface to toggle dry-run behavior.
Please confirm where DryRun is honored in the tx flow (e.g., skip Send and only record draft state). If not yet used, I can provide a small patch to gate fp.eth.Send on DryRun.
169-171: Stricter ChainId validation is goodEnsuring ChainId is non-nil and non-zero prevents invalid signer selection and tx failures.
| var wrappedConfig FeeConfigFileWrapper | ||
| if err := v.Unmarshal(&wrappedConfig); err != nil { | ||
| return fmt.Errorf("failed to unmarshal config: %w", err) | ||
| } | ||
|
|
||
| c.ChainId = big.NewInt(0).SetUint64(c.chainId) | ||
| // Copy all values from the wrapped config to the original config | ||
| *c = wrappedConfig.FeeConfig | ||
|
|
||
| c.ChainId = big.NewInt(0).SetUint64(wrappedConfig.ChainIdRaw) | ||
| return nil |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Avoid clobbering defaults when loading from file
Overwriting the entire FeeConfig with the unmarshalled struct will zero out unspecified fields, losing defaults established by withDefaults. Prefer merging into the existing config and only overriding provided keys.
Apply this diff:
- var wrappedConfig FeeConfigFileWrapper
- if err := v.Unmarshal(&wrappedConfig); err != nil {
- return fmt.Errorf("failed to unmarshal config: %w", err)
- }
-
- // Copy all values from the wrapped config to the original config
- *c = wrappedConfig.FeeConfig
-
- c.ChainId = big.NewInt(0).SetUint64(wrappedConfig.ChainIdRaw)
+ var wrappedConfig FeeConfigFileWrapper
+ if err := v.Unmarshal(&wrappedConfig); err != nil {
+ return fmt.Errorf("failed to unmarshal config: %w", err)
+ }
+ // Merge into existing defaults: only override fields provided in the file/env.
+ if err := v.Unmarshal(c); err != nil {
+ return fmt.Errorf("failed to unmarshal into FeeConfig: %w", err)
+ }
+ // Handle chain_id separately
+ if wrappedConfig.ChainIdRaw != 0 {
+ c.ChainId = big.NewInt(0).SetUint64(wrappedConfig.ChainIdRaw)
+ }This change preserves defaults. If your config files include all fields, behavior remains unchanged; otherwise, this prevents unexpected zero-values.
🏁 Script executed:
#!/bin/bash
set -e
# Locate all config.go files
echo "Config files in repo:"
fd -t f -e go config.go
# Inspect the plugin/fees/config.go content around unmarshalling
echo -e "\n--- Context around v.Unmarshal in plugin/fees/config.go ---"
rg -n "v.Unmarshal" -C5 plugin/fees/config.go
# Find definitions of FeeConfigFileWrapper and FeeConfig
echo -e "\n--- Definition of FeeConfigFileWrapper ---"
rg -n "type FeeConfigFileWrapper" -C5
echo -e "\n--- Definition of FeeConfig ---"
rg -n "type FeeConfig" -C5
# Search for withDefaults usage
echo -e "\n--- Occurrences of withDefaults ---"
rg -n "withDefaults" -C5Length of output: 4140
Merge file-loaded settings into existing defaults
Overwriting FeeConfig via *c = wrappedConfig.FeeConfig will zero out any fields not present in the file, dropping the defaults set by withDefaults. Instead, decode into the already-initialized struct so only provided keys override their defaults, and handle chain_id separately.
Locations to update:
- plugin/fees/config.go: lines 138–147
Proposed diff:
- var wrappedConfig FeeConfigFileWrapper
- if err := v.Unmarshal(&wrappedConfig); err != nil {
- return fmt.Errorf("failed to unmarshal config: %w", err)
- }
-
- // Copy all values from the wrapped config to the original config
- *c = wrappedConfig.FeeConfig
-
- c.ChainId = big.NewInt(0).SetUint64(wrappedConfig.ChainIdRaw)
- return nil
+ var wrappedConfig FeeConfigFileWrapper
+ if err := v.Unmarshal(&wrappedConfig); err != nil {
+ return fmt.Errorf("failed to unmarshal config: %w", err)
+ }
+ // Merge into existing defaults: only override provided fields
+ if err := v.Unmarshal(c); err != nil {
+ return fmt.Errorf("failed to unmarshal into FeeConfig: %w", err)
+ }
+ // Override chain_id only if set in the file
+ if wrappedConfig.ChainIdRaw != 0 {
+ c.ChainId = big.NewInt(0).SetUint64(wrappedConfig.ChainIdRaw)
+ }
+ return nil📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var wrappedConfig FeeConfigFileWrapper | |
| if err := v.Unmarshal(&wrappedConfig); err != nil { | |
| return fmt.Errorf("failed to unmarshal config: %w", err) | |
| } | |
| c.ChainId = big.NewInt(0).SetUint64(c.chainId) | |
| // Copy all values from the wrapped config to the original config | |
| *c = wrappedConfig.FeeConfig | |
| c.ChainId = big.NewInt(0).SetUint64(wrappedConfig.ChainIdRaw) | |
| return nil | |
| var wrappedConfig FeeConfigFileWrapper | |
| if err := v.Unmarshal(&wrappedConfig); err != nil { | |
| return fmt.Errorf("failed to unmarshal config: %w", err) | |
| } | |
| // Merge into existing defaults: only override provided fields | |
| if err := v.Unmarshal(c); err != nil { | |
| return fmt.Errorf("failed to unmarshal into FeeConfig: %w", err) | |
| } | |
| // Override chain_id only if set in the file | |
| if wrappedConfig.ChainIdRaw != 0 { | |
| c.ChainId = big.NewInt(0).SetUint64(wrappedConfig.ChainIdRaw) | |
| } | |
| return nil |
🤖 Prompt for AI Agents
In plugin/fees/config.go around lines 138–147, the code currently overwrites the
initialized FeeConfig with "*c = wrappedConfig.FeeConfig", which wipes defaults
set by withDefaults; instead, merge the file values into the existing struct so
only provided keys override defaults. Remove the full struct assignment and
copy/merge the fields from wrappedConfig.FeeConfig into *c (either
field-by-field or using a safe merge helper) so zero-value fields in the file do
not erase defaults, and still set c.ChainId explicitly via c.ChainId =
big.NewInt(0).SetUint64(wrappedConfig.ChainIdRaw).
| resourceName := "ethereum.erc20.transfer" | ||
| if rule.Resource != resourceName { | ||
| return nil, fmt.Errorf("rule resource expected to be ", resourceName) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Wrong resource constant and broken error formatting
- The policy is intentionally restricted to USDC transfers, not generic ERC-20. Per prior decisions, resource should be "ethereum.usdc.transfer".
- The fmt.Errorf call misses a format specifier and won’t display the expected value.
Apply this diff:
- resourceName := "ethereum.erc20.transfer"
- if rule.Resource != resourceName {
- return nil, fmt.Errorf("rule resource expected to be ", resourceName)
- }
+ resourceName := "ethereum.usdc.transfer"
+ if rule.Resource != resourceName {
+ return nil, fmt.Errorf("rule resource expected to be %s, got %s", resourceName, rule.Resource)
+ }Given previous learnings, please confirm policy.go still enforces/declares "ethereum.usdc.transfer". If not, we should align both places.
🏁 Script executed:
#!/bin/bash
set -e
echo "== Searching in plugin/fees/policy.go =="
rg -n '"ethereum\.(usdc|erc20)\.transfer"' -C2 plugin/fees/policy.go || true
echo "== Searching in plugin/fees/transaction.go =="
rg -n '"ethereum\.(usdc|erc20)\.transfer"' -C2 plugin/fees/transaction.go || trueLength of output: 710
Correct resource identifiers and error formatting
The plugin currently allows generic ERC-20 transfers and logs errors incorrectly. Both files need updating:
- plugin/fees/policy.go: change the full resource string from
"ethereum.erc20.transfer"to"ethereum.usdc.transfer". - plugin/fees/transaction.go: update the
resourceNameconstant to"ethereum.usdc.transfer"and fix thefmt.Errorfcall to include format specifiers.
Apply these diffs:
--- plugin/fees/policy.go
@@ -44,3 +44,3 @@
ProtocolId: "usdc",
FunctionId: "transfer",
- Full: "ethereum.erc20.transfer",
+ Full: "ethereum.usdc.transfer",--- plugin/fees/transaction.go
@@ -73,6 +73,7 @@
rule := recipe.Rules[0]
- resourceName := "ethereum.erc20.transfer"
+ resourceName := "ethereum.usdc.transfer"
if rule.Resource != resourceName {
- return nil, fmt.Errorf("rule resource expected to be ", resourceName)
+ return nil, fmt.Errorf("rule resource expected to be %s, got %s", resourceName, rule.Resource)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| resourceName := "ethereum.erc20.transfer" | |
| if rule.Resource != resourceName { | |
| return nil, fmt.Errorf("rule resource expected to be ", resourceName) | |
| } | |
| rule := recipe.Rules[0] | |
| resourceName := "ethereum.usdc.transfer" | |
| if rule.Resource != resourceName { | |
| return nil, fmt.Errorf("rule resource expected to be %s, got %s", resourceName, rule.Resource) | |
| } |
🤖 Prompt for AI Agents
In plugin/fees/transaction.go around lines 73 to 76, change the resourceName
constant from "ethereum.erc20.transfer" to "ethereum.usdc.transfer" and fix the
error construction so fmt.Errorf includes a format specifier and the resource
value (e.g., fmt.Errorf("rule resource expected to be %q, got %q", resourceName,
rule.Resource)); also update plugin/fees/policy.go where the full resource
string is declared to use "ethereum.usdc.transfer" instead of
"ethereum.erc20.transfer".
7cc0107 to
15eeafd
Compare
Small refactoring, adds dry run functionality into the config, adds target into the code
Summary by CodeRabbit
New Features
Bug Fixes