Fee code location move #158
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. WalkthroughAdds a DryRun flag and a FeeConfigFileWrapper for chain_id unmarshalling; introduces LoadFees with semaphore-limited concurrency to fetch/attach pending fees; implements HandleTransactions with concurrent run processing, propose/execute flows and signing orchestration; adds/changes public APIs (ValidateProposedTransactions, ProposeTransactions/SigningComplete stubs). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Scheduler as Job Scheduler
participant FP as FeePlugin
participant Repo as Repo/DB
participant Verifier as Verifier API
Scheduler->>FP: LoadFees(ctx, task)
activate FP
FP->>Repo: GetAllFeePolicies()
loop per policy (semaphore-limited)
FP->>Verifier: GetPublicKeysFees(policy.PublicKey)
alt fees pending
FP->>Repo: GetPendingFeeRun(policy.ID)
alt no draft run
FP->>Repo: CreateFeeRun(Draft)
end
FP->>Repo: CreateFee(fee) for each uncollected
else no fees
Note over FP: skip
end
end
deactivate FP
sequenceDiagram
autonumber
actor Scheduler as Job Scheduler
participant FP as FeePlugin
participant Repo as Repo/DB
participant Vault as Vault/Treasury
participant Sign as Sign Service
Scheduler->>FP: HandleTransactions(ctx, task)
activate FP
FP->>Repo: GetAllFeeRuns()
loop each eligible run (semaphore-limited)
FP->>Repo: GetPolicy(run.PolicyID)
FP->>Vault: Resolve sender addr, token (USDC), treasury addr
FP->>FP: proposeTransactions(policy, run)
alt proposals ready
loop each request
FP->>Sign: initSign(request)
end
else error
Note over FP: Log and handle per-run
end
end
deactivate FP
sequenceDiagram
autonumber
participant Caller as Caller
participant FP as FeePlugin
participant Engine as Recipe Engine
Caller->>FP: ValidateProposedTransactions(policy, txs)
activate FP
FP->>FP: ValidatePluginPolicy(policy)
loop each tx
FP->>FP: base64 decode tx.payload
FP->>Engine: Evaluate(recipe, chain, txBytes)
Engine-->>FP: result / error
end
FP-->>Caller: success / error
deactivate FP
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ 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 refactors the fee collection code by reorganizing it across multiple files for better modularity and maintainability. The main purpose is to separate concerns by moving transaction handling logic, fee loading logic, and deprecated methods into dedicated files.
- Moves transaction handling logic from the main fees.go file to transaction.go
- Extracts fee loading functionality into a new load.go file
- Relocates deprecated methods to helper.go
- Updates configuration handling to support dry run mode and improves chain ID validation
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| plugin/fees/transaction.go | Contains refactored transaction handling logic with new HandleTransactions and executeFeeTransaction methods |
| plugin/fees/load.go | New file containing fee loading functionality extracted from fees.go |
| plugin/fees/helper.go | Updated with deprecated method implementations moved from transaction.go |
| plugin/fees/fees.go | Simplified main file with transaction validation logic and reduced scope |
| plugin/fees/config.go | Enhanced configuration with dry run support and improved chain ID handling |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
plugin/fees/transaction.go
Outdated
| fp.logger.WithFields(logrus.Fields{ | ||
| "publicKey": feePolicy.PublicKey, | ||
| }).Info("Getting vault") | ||
| vaultFileName := vcommon.GetVaultBackupFilename(feePolicy.PublicKey, vtypes.PluginVultisigFees_feee.String()) |
There was a problem hiding this comment.
There's a typo in the enum value PluginVultisigFees_feee - it should likely be PluginVultisigFees_fee (with three 'e's instead of four).
| vaultFileName := vcommon.GetVaultBackupFilename(feePolicy.PublicKey, vtypes.PluginVultisigFees_feee.String()) | |
| vaultFileName := vcommon.GetVaultBackupFilename(feePolicy.PublicKey, vtypes.PluginVultisigFees_fee.String()) |
![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
🧹 Nitpick comments (4)
plugin/fees/config.go (1)
39-42: Consider documenting the wrapper structure's purpose.The
FeeConfigFileWrapperserves as an intermediate structure to handle the rawchain_idunmarshalling. Consider adding a comment to explain why this wrapper pattern is necessary (i.e., to separate the raw uint64 chain_id value from the final *big.Int representation).+// FeeConfigFileWrapper is used to unmarshal the config file, handling the raw chain_id +// as uint64 before converting it to *big.Int in the main FeeConfig struct type FeeConfigFileWrapper struct { FeeConfig `mapstructure:",squash"` ChainIdRaw uint64 `mapstructure:"chain_id"` }plugin/fees/load.go (1)
40-51: Potential goroutine leak on context cancellation.The goroutine launched in the errgroup doesn't check for context cancellation before acquiring the semaphore. If the context is cancelled while waiting to acquire, the goroutine might continue running unnecessarily.
Consider checking context before acquiring:
eg.Go(func() error { defer wg.Done() + select { + case <-ctx.Done(): + return ctx.Err() + default: + } if err := sem.Acquire(ctx, 1); err != nil { return fmt.Errorf("failed to acquire semaphore: %w", err) }plugin/fees/transaction.go (2)
262-265: Consider using the configured resource from policy.The resource name is hardcoded as "ethereum.erc20.transfer" but should ideally be validated against a constant or configuration, especially given the learning that fees are only collected in USDC.
Consider defining this as a constant:
+const FeeTransferResource = "ethereum.erc20.transfer" + resourceName := "ethereum.erc20.transfer" if rule.Resource != resourceName { return nil, fmt.Errorf("rule resource expected to be %s", resourceName) }
296-302: Amount conversion may cause overflow for large values.Converting
run.TotalAmount(int) toint64could potentially overflow for very large fee amounts. Consider validating the range or using big.Int throughout.-amount := run.TotalAmount +amount := big.NewInt(0).SetInt64(int64(run.TotalAmount)) +if amount.Cmp(big.NewInt(0)) <= 0 { + return nil, fmt.Errorf("invalid fee amount: %d", run.TotalAmount) +} tx, err := fp.eth.MakeAnyTransfer(ctx, gcommon.HexToAddress(ethAddress), gcommon.HexToAddress(recipient), gcommon.HexToAddress(usdc.Address), - big.NewInt(int64(amount))) + amount)
📜 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 (5)
plugin/fees/config.go(6 hunks)plugin/fees/fees.go(2 hunks)plugin/fees/helper.go(3 hunks)plugin/fees/load.go(1 hunks)plugin/fees/transaction.go(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
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.
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.
📚 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/helper.goplugin/fees/load.goplugin/fees/fees.goplugin/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/helper.goplugin/fees/fees.goplugin/fees/transaction.go
📚 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/load.goplugin/fees/fees.goplugin/fees/config.goplugin/fees/transaction.go
📚 Learning: 2025-05-07T08:23:45.882Z
Learnt from: RaghavSood
PR: vultisig/plugin#36
File: api/server.go:21-33
Timestamp: 2025-05-07T08:23:45.882Z
Learning: The import path `github.com/vultisig/verifier/plugin` refers to an external dependency that provides the plugin interface, and should not be changed to `github.com/vultisig/plugin/plugin` as these are distinct packages with different purposes.
Applied to files:
plugin/fees/fees.go
🧬 Code Graph Analysis (4)
plugin/fees/helper.go (2)
plugin/fees/fees.go (1)
FeePlugin(33-48)internal/types/keysign.go (1)
PluginKeysignRequest(7-12)
plugin/fees/load.go (2)
plugin/fees/fees.go (1)
FeePlugin(33-48)internal/types/fees.go (1)
FeeRunStateDraft(14-14)
plugin/fees/fees.go (1)
internal/types/keysign.go (1)
PluginKeysignRequest(7-12)
plugin/fees/transaction.go (4)
plugin/fees/fees.go (1)
FeePlugin(33-48)internal/types/fees.go (2)
FeeRunStateDraft(14-14)FeeRun(29-39)internal/types/keysign.go (1)
PluginKeysignRequest(7-12)common/vault.go (1)
GetVaultFromPolicy(12-24)
🔇 Additional comments (7)
plugin/fees/config.go (1)
169-171: Good validation improvement for ChainId.The validation now correctly checks for both nil and zero values for ChainId, ensuring it's properly initialized.
plugin/fees/helper.go (2)
121-124: Intentional placeholder aligns with fee plugin v2 architecture.The ProposeTransactions method returning "not implemented" is intentional, as confirmed by the learning about the fees plugin v2's run-based architecture. The actual transaction proposal logic is in the private
proposeTransactionsmethod that accepts atypes.FeeRunparameter.
126-129: SigningComplete deprecation aligns with new architecture.The SigningComplete method is correctly marked as deprecated with "not implemented" since the new architecture handles signing through
initSignand post-transaction flow, as mentioned in the comment.plugin/fees/load.go (1)
79-87: Good validation of fee amounts.The code properly validates that the sum of uncollected fees matches the
FeesPendingCollectionamount, preventing discrepancies in fee accounting.plugin/fees/fees.go (1)
109-142: Well-structured policy-based validation flow.The ValidateProposedTransactions method properly validates both the policy and each transaction using the recipe engine. The sequential validation approach ensures all transactions comply with the policy constraints.
plugin/fees/transaction.go (2)
41-106: Robust concurrent transaction handling with proper mutex control.The HandleTransactions method correctly serializes operations with a mutex and processes fee runs concurrently with proper semaphore-based concurrency control. The lifecycle logging will help with debugging and monitoring.
210-213: Critical error handling for transaction state recording.Excellent error handling here. The comment correctly highlights the importance of recording the transaction state to prevent internal state inconsistency after broadcast.
garry-sharp
force-pushed
the
fee-code-location-move
branch
2 times, most recently
from
30700c0 to
4caaa4e
Compare
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)
plugin/fees/fees.go (1)
115-148: Engine validation is decoding the wrong field; use RawMessage hex, not Message (hash).keysigned Message is the keccak sign-hash (base64) while RawMessage carries the unsigned tx payload (hex). Passing the hash to the recipe engine will cause malformed/unevaluable input. Also, prefer %w for error wrapping.
Apply this diff:
@@ func (fp *FeePlugin) ValidateProposedTransactions(policy vtypes.PluginPolicy, txs []vtypes.PluginKeysignRequest) error { // First validate the plugin policy itself err := fp.ValidatePluginPolicy(policy) if err != nil { - return fmt.Errorf("failed to validate plugin policy: %v", err) + return fmt.Errorf("failed to validate plugin policy: %w", err) } @@ recipe, err := policy.GetRecipe() if err != nil { - return fmt.Errorf("failed to get recipe from policy: %v", err) + return fmt.Errorf("failed to get recipe from policy: %w", err) } @@ - for _, tx := range txs { - for _, keysignMessage := range tx.Messages { - txBytes, err := base64.StdEncoding.DecodeString(keysignMessage.Message) + for _, tx := range txs { + for _, keysignMessage := range tx.Messages { + // Validate the actual unsigned transaction bytes + txBytes, err := hexutil.Decode(keysignMessage.RawMessage) if err != nil { - return fmt.Errorf("failed to decode transaction: %w", err) + return fmt.Errorf("failed to decode raw transaction: %w", err) } // Evaluate if the transaction is allowed by the policy _, err = eng.Evaluate(recipe, vgcommon.Chain(keysignMessage.Chain), txBytes) if err != nil { return fmt.Errorf("failed to evaluate transaction: %w", err) } } }And ensure the import exists at the top of this file:
+ "github.com/ethereum/go-ethereum/common/hexutil"
🧹 Nitpick comments (3)
plugin/fees/transaction.go (3)
40-105: Redundant/misused WaitGroup; errgroup already handles lifecycle.wg.Add is called inside the goroutine and wg.Wait() runs before eg.Wait(); this WaitGroup provides no ordering guarantees and can be dropped. Keep concurrency limiting via semaphore and the errgroup for error propagation.
Apply this diff:
@@ - sem := semaphore.NewWeighted(int64(fp.config.Jobs.Transact.MaxConcurrentJobs)) - var wg sync.WaitGroup + sem := semaphore.NewWeighted(int64(fp.config.Jobs.Transact.MaxConcurrentJobs)) var eg errgroup.Group for _, run := range runs { run = run eg.Go(func() error { @@ feePolicy, err := fp.db.GetPluginPolicy(ctx, run.PolicyID) if err != nil { return fmt.Errorf("failed to get fee policy: %w", err) } - wg.Add(1) fp.logger.WithFields(logrus.Fields{"runId": run.ID, "policyId": run.PolicyID}).Info("Retrieved fee policy") - defer wg.Done() if err := sem.Acquire(ctx, 1); err != nil { return fmt.Errorf("failed to acquire semaphore: %w", err) } defer sem.Release(1) @@ }) } - wg.Wait() if err := eg.Wait(); err != nil { return fmt.Errorf("failed to execute fee transaction: %w", err) }If you adopt this, remove the now-unused
syncimport from this file.
63-75: Consider handling retryable “failed” runs too.You have a TODO; if a run previously failed after signing/broadcast hiccups, it will never be retried. Consider explicit states and backoff. I can sketch a small state machine if helpful.
121-133: Avoid double vault retrieval/decrypt.You ensure vault presence here, then
proposeTransactionsfetches/decrypts the vault again. Pass the vault down to avoid duplicate IO/decrypt.Example signature change:
-func (fp *FeePlugin) executeFeeTransaction(ctx context.Context, run types.FeeRun, feePolicy vtypes.PluginPolicy) error { +func (fp *FeePlugin) executeFeeTransaction(ctx context.Context, run types.FeeRun, feePolicy vtypes.PluginPolicy) error { @@ - _, err := common.GetVaultFromPolicy(fp.vaultStorage, feePolicy, fp.encryptionSecret) + vault, err := common.GetVaultFromPolicy(fp.vaultStorage, feePolicy, fp.encryptionSecret) @@ - keySignRequests, err := fp.proposeTransactions(ctx, feePolicy, run) + keySignRequests, err := fp.proposeTransactions(ctx, feePolicy, run, vault)And update
proposeTransactionsto accept the vault without re-fetching.
📜 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 (4)
plugin/fees/fees.go(2 hunks)plugin/fees/helper.go(3 hunks)plugin/fees/load.go(1 hunks)plugin/fees/transaction.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- plugin/fees/helper.go
- plugin/fees/load.go
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
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-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.goplugin/fees/fees.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.goplugin/fees/fees.go
📚 Learning: 2025-07-10T20:41:44.025Z
Learnt from: webpiratt
PR: vultisig/plugin#126
File: plugin/fees/policy.go:26-26
Timestamp: 2025-07-10T20:41:44.025Z
Learning: The constant `vtypes.PluginVultisigFees_feee` in the vultisig/plugin project is correctly spelled with "feee" (including the extra 'e'). This is the actual constant name defined in the external dependency github.com/vultisig/verifier/types and should not be changed to "fee" singular, as this is how it's defined in the external package.
Applied to files:
plugin/fees/transaction.go
📚 Learning: 2025-07-04T10:50:42.832Z
Learnt from: garry-sharp
PR: vultisig/plugin#117
File: plugin/fees/constraints.go:3-3
Timestamp: 2025-07-04T10:50:42.832Z
Learning: For the vultisig/plugin project, use "fee" (singular) throughout the codebase wherever possible, not "fees" (plural). This applies to all naming conventions including plugin types, variable names, function names, comments, and documentation.
Applied to files:
plugin/fees/transaction.go
📚 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/fees.go
📚 Learning: 2025-08-20T13:51:05.194Z
Learnt from: neavra
PR: vultisig/plugin#157
File: api/plugin.go:236-236
Timestamp: 2025-08-20T13:51:05.194Z
Learning: In the vultisig/plugin codebase migration to vultisig-go, SystemMigrations() and vultisig_validator functionality were not moved from the verifier package to vultisig-go. This means some imports of github.com/vultisig/verifier/common are intentionally retained for accessing these specific functions, while vault-related types and common functionality were migrated to vultisig-go.
Applied to files:
plugin/fees/fees.go
📚 Learning: 2025-08-20T13:51:41.769Z
Learnt from: neavra
PR: vultisig/plugin#157
File: api/server.go:22-24
Timestamp: 2025-08-20T13:51:41.769Z
Learning: In the vultisig-go migration, only vault-related functions (GetVaultBackupFilename, DecryptVaultFromBackup) were moved from github.com/vultisig/verifier/common to github.com/vultisig/vultisig-go/common. Other functions like SystemMigrations() remained in the verifier/common package, so files using these non-migrated functions should keep the original verifier/common import.
Applied to files:
plugin/fees/fees.go
🧬 Code Graph Analysis (2)
plugin/fees/transaction.go (4)
plugin/fees/fees.go (1)
FeePlugin(34-49)internal/types/fees.go (2)
FeeRunStateDraft(14-14)FeeRun(29-39)common/vault.go (1)
GetVaultFromPolicy(12-24)internal/types/keysign.go (1)
PluginKeysignRequest(7-12)
plugin/fees/fees.go (1)
internal/types/keysign.go (1)
PluginKeysignRequest(7-12)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
plugin/fees/transaction.go (2)
107-116: Policy/run alignment check is solid.Good early guard to avoid cross-policy leakage.
147-214: Helpers confirmed; remove stale TODOI’ve verified that all three helpers live in
plugin/fees/helper.go(lines 21–23, 46–48, 113–115), so there’s no missing implementation. The// TODO pass the real tx idatplugin/fees/transaction.go:206is now stale—tx.Hash().Hex()already provides the correct identifier.• Remove the
// TODO pass the real tx idcomment on line 206 ofplugin/fees/transaction.go.
• (Optional) Ensure there are unit tests coveringhexutilDecode,getHash, anddecodeTxfor the EVM flavor you’re targeting.
garry-sharp
force-pushed
the
fee-code-location-move
branch
from
4caaa4e to
09f218e
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
plugin/fees/transaction.go (1)
225-233: Use runtime chain ID for USDC token (avoid hard-coded 1)Hard-coding ChainId=1 breaks non-mainnet deployments. Use config.
Apply this diff:
// ERC20 USDC Token List var usdc *reth.Token = &reth.Token{ - ChainId: 1, + ChainId: int(fp.config.ChainId.Int64()), Address: fp.config.UsdcAddress, Name: "USD Coin", Symbol: "USDC", Decimals: 6, }
🧹 Nitpick comments (2)
plugin/fees/fees.go (1)
3-14: Update imports: drop base64, add hexutilThe refactor above requires hex decoding and no longer uses base64 here.
Apply this diff:
@@ -import ( - "encoding/base64" +import ( "fmt" "sync" @@ "github.com/ethereum/go-ethereum/ethclient" + "github.com/ethereum/go-ethereum/common/hexutil"plugin/fees/transaction.go (1)
79-87: Remove redundant WaitGroup and use the cancelable context for semaphore AcquireThe WaitGroup duplicates errgroup.Wait(). Also pass gctx to semaphore to honor cancellation when a sibling errors.
Apply this diff:
- wg.Add(1) fp.logger.WithFields(logrus.Fields{"runId": run.ID, "policyId": run.PolicyID}).Info("Retrieved fee policy") - - defer wg.Done() - if err := sem.Acquire(ctx, 1); err != nil { + if err := sem.Acquire(gctx, 1); err != nil { return fmt.Errorf("failed to acquire semaphore: %w", err) }And at the end of the function:
- wg.Wait() - if err := eg.Wait(); err != nil { + if err := eg.Wait(); err != nil { return fmt.Errorf("failed to execute fee transaction: %w", err) }Also remove the now-unused
syncimport if nothing else uses it at the top of this file.
📜 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 (5)
plugin/fees/config.go(6 hunks)plugin/fees/fees.go(2 hunks)plugin/fees/helper.go(3 hunks)plugin/fees/load.go(1 hunks)plugin/fees/transaction.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- plugin/fees/helper.go
- plugin/fees/load.go
- plugin/fees/config.go
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
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-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/fees.goplugin/fees/transaction.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/fees.goplugin/fees/transaction.go
📚 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/fees.goplugin/fees/transaction.go
📚 Learning: 2025-08-20T13:51:05.212Z
Learnt from: neavra
PR: vultisig/plugin#157
File: api/plugin.go:236-236
Timestamp: 2025-08-20T13:51:05.212Z
Learning: In the vultisig/plugin codebase migration to vultisig-go, SystemMigrations() and vultisig_validator functionality were not moved from the verifier package to vultisig-go. This means some imports of github.com/vultisig/verifier/common are intentionally retained for accessing these specific functions, while vault-related types and common functionality were migrated to vultisig-go.
Applied to files:
plugin/fees/fees.go
📚 Learning: 2025-05-07T08:23:45.882Z
Learnt from: RaghavSood
PR: vultisig/plugin#36
File: api/server.go:21-33
Timestamp: 2025-05-07T08:23:45.882Z
Learning: The import path `github.com/vultisig/verifier/plugin` refers to an external dependency that provides the plugin interface, and should not be changed to `github.com/vultisig/plugin/plugin` as these are distinct packages with different purposes.
Applied to files:
plugin/fees/fees.go
📚 Learning: 2025-08-20T13:51:41.822Z
Learnt from: neavra
PR: vultisig/plugin#157
File: api/server.go:22-24
Timestamp: 2025-08-20T13:51:41.822Z
Learning: In the vultisig-go migration, only vault-related functions (GetVaultBackupFilename, DecryptVaultFromBackup) were moved from github.com/vultisig/verifier/common to github.com/vultisig/vultisig-go/common. Other functions like SystemMigrations() remained in the verifier/common package, so files using these non-migrated functions should keep the original verifier/common import.
Applied to files:
plugin/fees/fees.go
📚 Learning: 2025-07-10T20:41:44.025Z
Learnt from: webpiratt
PR: vultisig/plugin#126
File: plugin/fees/policy.go:26-26
Timestamp: 2025-07-10T20:41:44.025Z
Learning: The constant `vtypes.PluginVultisigFees_feee` in the vultisig/plugin project is correctly spelled with "feee" (including the extra 'e'). This is the actual constant name defined in the external dependency github.com/vultisig/verifier/types and should not be changed to "fee" singular, as this is how it's defined in the external package.
Applied to files:
plugin/fees/transaction.go
📚 Learning: 2025-07-04T10:50:42.832Z
Learnt from: garry-sharp
PR: vultisig/plugin#117
File: plugin/fees/constraints.go:3-3
Timestamp: 2025-07-04T10:50:42.832Z
Learning: For the vultisig/plugin project, use "fee" (singular) throughout the codebase wherever possible, not "fees" (plural). This applies to all naming conventions including plugin types, variable names, function names, comments, and documentation.
Applied to files:
plugin/fees/transaction.go
🔇 Additional comments (2)
plugin/fees/transaction.go (2)
289-296: The commands are queued for execution.
260-270: Remove suggestion to use non-existent GetMagicConstant()
Constraint has noGetMagicConstant()method; continue usingGetFixedValue()and parsing its string output.Likely an incorrect or invalid review comment.
| sem := semaphore.NewWeighted(int64(fp.config.Jobs.Transact.MaxConcurrentJobs)) | ||
| var wg sync.WaitGroup | ||
| var eg errgroup.Group | ||
| for _, run := range runs { | ||
| run = run | ||
| eg.Go(func() error { | ||
| //TODO also check failed runs |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use errgroup.WithContext to enable cancellation of siblings
An error in one worker should cancel others. Plain errgroup.Group doesn’t propagate cancellation. Use WithContext.
Apply this diff:
- sem := semaphore.NewWeighted(int64(fp.config.Jobs.Transact.MaxConcurrentJobs))
- var wg sync.WaitGroup
- var eg errgroup.Group
+ sem := semaphore.NewWeighted(int64(fp.config.Jobs.Transact.MaxConcurrentJobs))
+ eg, gctx := errgroup.WithContext(ctx)📝 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.
| sem := semaphore.NewWeighted(int64(fp.config.Jobs.Transact.MaxConcurrentJobs)) | |
| var wg sync.WaitGroup | |
| var eg errgroup.Group | |
| for _, run := range runs { | |
| run = run | |
| eg.Go(func() error { | |
| //TODO also check failed runs | |
| sem := semaphore.NewWeighted(int64(fp.config.Jobs.Transact.MaxConcurrentJobs)) | |
| eg, gctx := errgroup.WithContext(ctx) | |
| for _, run := range runs { | |
| run = run | |
| eg.Go(func() error { | |
| //TODO also check failed runs |
🤖 Prompt for AI Agents
In plugin/fees/transaction.go around lines 55 to 61, replace the plain
errgroup.Group with errgroup.WithContext so a failure in one worker cancels the
others: obtain a parent context (use the surrounding function's ctx if
available, otherwise context.Background()), call ctx, eg :=
errgroup.WithContext(parentCtx), and then in each goroutine use that ctx (e.g.,
sem.Acquire(ctx, 1) and check ctx for cancellation) and return errors via the eg
goroutine; keep using eg.Wait() to propagate the first error and ensure
sem.Release is deferred inside the goroutine.
| for _, run := range runs { | ||
| run = run | ||
| eg.Go(func() error { | ||
| //TODO also check failed runs | ||
| if run.Status != types.FeeRunStateDraft { | ||
| return nil | ||
| } | ||
|
|
||
| txs := []vtypes.PluginKeysignRequest{} | ||
| var magicConstantRecipientValue rtypes.MagicConstant = rtypes.MagicConstant_UNSPECIFIED | ||
| var token string | ||
| // 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. | ||
| for _, rule := range recipe.Rules { | ||
|
|
||
| // This section of code goes through the rules in the fee policy. It looks for the recipient of the fee collection policy and extracts it. If other data is found throws an error as they're unsupported rules. | ||
| var recipient string // The address specified in the fee policy. | ||
| switch rule.Resource { | ||
| case "ethereum.erc20.transfer": | ||
| 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) | ||
| } | ||
| if run.TxHash != nil { |
There was a problem hiding this comment.
Fix goroutine capture of loop variable ‘run’
run = run is a no-op; the closure captures the reused range variable leading to wrong runs being processed.
Apply this diff:
- for _, run := range runs {
- run = run
- eg.Go(func() error {
+ for _, run := range runs {
+ r := run
+ eg.Go(func() error {
+ run := r📝 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.
| for _, run := range runs { | |
| run = run | |
| eg.Go(func() error { | |
| //TODO also check failed runs | |
| if run.Status != types.FeeRunStateDraft { | |
| return nil | |
| } | |
| txs := []vtypes.PluginKeysignRequest{} | |
| var magicConstantRecipientValue rtypes.MagicConstant = rtypes.MagicConstant_UNSPECIFIED | |
| var token string | |
| // 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. | |
| for _, rule := range recipe.Rules { | |
| // This section of code goes through the rules in the fee policy. It looks for the recipient of the fee collection policy and extracts it. If other data is found throws an error as they're unsupported rules. | |
| var recipient string // The address specified in the fee policy. | |
| switch rule.Resource { | |
| case "ethereum.erc20.transfer": | |
| 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) | |
| } | |
| if run.TxHash != nil { | |
| // Before: for _, run := range runs { | |
| // run = run | |
| // eg.Go(func() error { | |
| // After: | |
| for _, run := range runs { | |
| // capture the current iteration’s value | |
| r := run | |
| eg.Go(func() error { | |
| // shadow run inside the closure so each goroutine sees its own copy | |
| run := r | |
| // TODO also check failed runs | |
| if run.Status != types.FeeRunStateDraft { | |
| return nil | |
| } | |
| if run.TxHash != nil { | |
| // existing logic… | |
| } | |
| // … | |
| }) | |
| } |
🤖 Prompt for AI Agents
In plugin/fees/transaction.go around lines 58 to 66, the goroutine closure
captures the reused range variable `run` (the existing `run = run` is a no-op)
causing incorrect runs to be processed; fix by creating a local copy inside the
loop (e.g. `r := run`) and use that local `r` inside the eg.Go closure or
alternatively pass `run` as an argument to the closure so each goroutine gets
its own copy; remove the pointless `run = run` and update all references in the
closure to use the new local variable.
09f218e to
e7ef35f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
plugin/fees/helper.go (3)
67-70: Fix potential panic when rawBytes is empty.On empty input,
rawBytes[0]in the error formatting will index out of range. Guard before indexing.- // Check transaction type (EIP-1559 is 0x02) - if len(rawBytes) == 0 || rawBytes[0] != 0x02 { - return nil, fmt.Errorf("unsupported transaction type: 0x%02x", rawBytes[0]) - } + // Check transaction type (EIP-1559 is 0x02) + if len(rawBytes) == 0 { + return nil, fmt.Errorf("unsupported transaction type: empty payload") + } + if rawBytes[0] != 0x02 { + return nil, fmt.Errorf("unsupported transaction type: 0x%02x", rawBytes[0]) + }
85-95: Add length checks before slicing and improve error wrapping.
tx.Data[:4]panics iflen(tx.Data) < 4. Also wrap underlying errors for diagnostics.- // Get the method by selector - method, err := parsedABI.MethodById(tx.Data[:4]) - if err != nil { - return nil, fmt.Errorf("unknown method ID") - } + // Get the method by selector + if len(tx.Data) < 4 { + return nil, fmt.Errorf("tx data too short for method selector: %d bytes", len(tx.Data)) + } + method, err := parsedABI.MethodById(tx.Data[:4]) + if err != nil { + return nil, fmt.Errorf("unknown method ID: %w", err) + } @@ - if err := method.Inputs.UnpackIntoMap(args, tx.Data[4:]); err != nil { - return nil, fmt.Errorf("failed get recipient and amount from tx") - } + if err := method.Inputs.UnpackIntoMap(args, tx.Data[4:]); err != nil { + return nil, fmt.Errorf("failed to unpack transfer args: %w", err) + }
107-111: Nil To-address check before dereference.
*tx.Towill panic iftx.To == nil(e.g., malformed payload). Guard it.- return &erc20tx{ - to: recipient, - amount: amount, - token: *tx.To, - }, nil + if tx.To == nil { + return nil, fmt.Errorf("nil token contract address in tx") + } + return &erc20tx{ + to: recipient, + amount: amount, + token: *tx.To, + }, nilplugin/fees/fees.go (1)
114-147: Validate the unsigned transaction payload, not the EIP-155 hash bytes.Engine should evaluate the raw unsigned tx (hex) once per request. Prefer
req.Transactionor fall back toMessages[0].RawMessage.@@ -func (fp *FeePlugin) ValidateProposedTransactions(policy vtypes.PluginPolicy, txs []vtypes.PluginKeysignRequest) error { - // First validate the plugin policy itself - err := fp.ValidatePluginPolicy(policy) - if err != nil { - return fmt.Errorf("failed to validate plugin policy: %v", err) - } +func (fp *FeePlugin) ValidateProposedTransactions(policy vtypes.PluginPolicy, txs []vtypes.PluginKeysignRequest) error { + // First validate the plugin policy itself + if err := fp.ValidatePluginPolicy(policy); err != nil { + return fmt.Errorf("failed to validate plugin policy: %w", err) + } @@ - recipe, err := policy.GetRecipe() - if err != nil { - return fmt.Errorf("failed to get recipe from policy: %v", err) - } + recipe, err := policy.GetRecipe() + if err != nil { + return fmt.Errorf("failed to get recipe from policy: %w", err) + } @@ - // Validate each proposed transaction - for _, tx := range txs { - for _, keysignMessage := range tx.Messages { - txBytes, err := base64.StdEncoding.DecodeString(keysignMessage.Message) - if err != nil { - return fmt.Errorf("failed to decode transaction: %w", err) - } - - // Evaluate if the transaction is allowed by the policy - _, err = eng.Evaluate(recipe, vgcommon.Chain(keysignMessage.Chain), txBytes) - if err != nil { - return fmt.Errorf("failed to evaluate transaction: %w", err) - } - } - } + // Validate each proposed transaction request + for _, req := range txs { + // Prefer raw unsigned tx hex; fall back to the first message's RawMessage + var rawHex string + switch { + case req.Transaction != "": + rawHex = req.Transaction + case len(req.Messages) > 0 && req.Messages[0].RawMessage != "": + rawHex = req.Messages[0].RawMessage + default: + return fmt.Errorf("missing raw unsigned transaction in request") + } + + rawBytes, err := hexutil.Decode(rawHex) + if err != nil { + return fmt.Errorf("failed to hex-decode raw tx: %w", err) + } + + chain := vgcommon.Ethereum + if len(req.Messages) > 0 { + chain = vgcommon.Chain(req.Messages[0].Chain) + } + if _, err := eng.Evaluate(recipe, chain, rawBytes); err != nil { + return fmt.Errorf("failed to evaluate transaction: %w", err) + } + } return nil }Add import:
@@ -import ( - "encoding/base64" +import ( + "encoding/base64" + "github.com/ethereum/go-ethereum/common/hexutil"
♻️ Duplicate comments (4)
plugin/fees/helper.go (1)
121-129: Stubs match the v2 run-based flow.Keeping ProposeTransactions/SigningComplete as deprecated stubs aligns with the run-based architecture; good to keep for compatibility.
plugin/fees/transaction.go (3)
255-258: Resource must match USDC-only policy.Policy enforces “ethereum.usdc.transfer”, not the generic ERC20 resource.
- resourceName := "ethereum.erc20.transfer" + resourceName := "ethereum.usdc.transfer"
58-61: Fix goroutine loop variable capture; remove no-op assignment.The closure captures the reused
runvariable;run = rundoes nothing. This causes incorrect runs to be processed.- for _, run := range runs { - run = run - eg.Go(func() error { + for _, run := range runs { + r := run + eg.Go(func() error { + run := r
225-232: Avoid hard-coded ChainId; use configured chain ID.Hard-coding
1breaks non-mainnet deployments.- var usdc *reth.Token = &reth.Token{ - ChainId: 1, + var usdc *reth.Token = &reth.Token{ + ChainId: int(fp.config.ChainId.Int64()),
🧹 Nitpick comments (5)
plugin/fees/transaction.go (5)
55-99: Use errgroup.WithContext and drop the extra WaitGroup.Let failures cancel sibling goroutines; the standalone WaitGroup is redundant with
eg.Wait().- sem := semaphore.NewWeighted(int64(fp.config.Jobs.Transact.MaxConcurrentJobs)) - var wg sync.WaitGroup - var eg errgroup.Group + sem := semaphore.NewWeighted(int64(fp.config.Jobs.Transact.MaxConcurrentJobs)) + eg, gctx := errgroup.WithContext(ctx) @@ - wg.Add(1) - fp.logger.WithFields(logrus.Fields{"runId": run.ID, "policyId": run.PolicyID}).Info("Retrieved fee policy") - - defer wg.Done() - if err := sem.Acquire(ctx, 1); err != nil { + fp.logger.WithFields(logrus.Fields{"runId": run.ID, "policyId": run.PolicyID}).Info("Retrieved fee policy") + if err := sem.Acquire(gctx, 1); err != nil { return fmt.Errorf("failed to acquire semaphore: %w", err) } defer sem.Release(1) - if err := fp.executeFeeTransaction(ctx, run, *feePolicy); err != nil { + if err := fp.executeFeeTransaction(gctx, run, *feePolicy); err != nil { fp.logger.WithError(err).WithFields(logrus.Fields{ "runId": run.ID, }).Error("Failed to execute fee transaction") return err } return nil }) } - wg.Wait() if err := eg.Wait(); err != nil { return fmt.Errorf("failed to execute fee transaction: %w", err) }
260-270: Prefer using the magic-constant getter over parsing strings.If available in the generated types, use
GetMagicConstant()to avoid brittlestrconv.ParseIntonFixedValue.- 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) + // If supported by the generated API: + magicConstantRecipientValue = rtypes.MagicConstant(constraint.Constraint.GetMagicConstant())If the getter is not available, keep existing logic.
309-327: Populate wrapper PolicyID for downstream JSON consumers.
PluginKeysignRequestalso has a top-levelPolicyID stringused in JSON. Set it to avoid relying solely on the embedded field.signRequest := vtypes.PluginKeysignRequest{ KeysignRequest: vtypes.KeysignRequest{ @@ PluginID: policy.PluginID.String(), }, Transaction: txHex, + PolicyID: policy.ID, // or policy.ID.String() if type is uuid.UUID }Also consider setting
TransactionTypeif consumers expect it.
183-187: Make decodeTx logging best-effort to avoid blocking broadcast.A failure to decode for logging shouldn’t abort a successfully signed/broadcastable tx. Log the error and proceed.
- erc20tx, err := decodeTx(req.Transaction) - if err != nil { - fp.logger.WithError(err).Error("failed to decode tx") - return fmt.Errorf("failed to decode tx: %w", err) - } + erc20tx, err := decodeTx(req.Transaction) + if err != nil { + fp.logger.WithError(err).Warn("failed to decode tx for logging; continuing") + }
203-206: Ensure idempotency if DB update fails after broadcast.If
SetFeeRunSentfails post-send, the run remains draft and may be re-broadcast. Consider persisting a “pending/broadcast” marker with raw tx or nonce before send, or reconciling by tx nonce/hash to prevent duplicates.
📜 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 (5)
plugin/fees/config.go(6 hunks)plugin/fees/fees.go(2 hunks)plugin/fees/helper.go(3 hunks)plugin/fees/load.go(1 hunks)plugin/fees/transaction.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- plugin/fees/config.go
- plugin/fees/load.go
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
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-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/helper.goplugin/fees/fees.goplugin/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/helper.goplugin/fees/fees.goplugin/fees/transaction.go
📚 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/fees.goplugin/fees/transaction.go
📚 Learning: 2025-06-18T18:23:20.077Z
Learnt from: webpiratt
PR: vultisig/plugin#96
File: plugin/payroll/transaction.go:0-0
Timestamp: 2025-06-18T18:23:20.077Z
Learning: In the payroll plugin (plugin/payroll/transaction.go), signRequest.Transaction is stored with the "0x" prefix, making it compatible with gcommon.FromHex which requires 0x-prefixed hex strings.
Applied to files:
plugin/fees/fees.go
📚 Learning: 2025-08-20T13:51:05.212Z
Learnt from: neavra
PR: vultisig/plugin#157
File: api/plugin.go:236-236
Timestamp: 2025-08-20T13:51:05.212Z
Learning: In the vultisig/plugin codebase migration to vultisig-go, SystemMigrations() and vultisig_validator functionality were not moved from the verifier package to vultisig-go. This means some imports of github.com/vultisig/verifier/common are intentionally retained for accessing these specific functions, while vault-related types and common functionality were migrated to vultisig-go.
Applied to files:
plugin/fees/fees.go
📚 Learning: 2025-05-07T08:23:45.882Z
Learnt from: RaghavSood
PR: vultisig/plugin#36
File: api/server.go:21-33
Timestamp: 2025-05-07T08:23:45.882Z
Learning: The import path `github.com/vultisig/verifier/plugin` refers to an external dependency that provides the plugin interface, and should not be changed to `github.com/vultisig/plugin/plugin` as these are distinct packages with different purposes.
Applied to files:
plugin/fees/fees.go
📚 Learning: 2025-08-20T13:51:41.822Z
Learnt from: neavra
PR: vultisig/plugin#157
File: api/server.go:22-24
Timestamp: 2025-08-20T13:51:41.822Z
Learning: In the vultisig-go migration, only vault-related functions (GetVaultBackupFilename, DecryptVaultFromBackup) were moved from github.com/vultisig/verifier/common to github.com/vultisig/vultisig-go/common. Other functions like SystemMigrations() remained in the verifier/common package, so files using these non-migrated functions should keep the original verifier/common import.
Applied to files:
plugin/fees/fees.go
📚 Learning: 2025-07-10T20:41:44.025Z
Learnt from: webpiratt
PR: vultisig/plugin#126
File: plugin/fees/policy.go:26-26
Timestamp: 2025-07-10T20:41:44.025Z
Learning: The constant `vtypes.PluginVultisigFees_feee` in the vultisig/plugin project is correctly spelled with "feee" (including the extra 'e'). This is the actual constant name defined in the external dependency github.com/vultisig/verifier/types and should not be changed to "fee" singular, as this is how it's defined in the external package.
Applied to files:
plugin/fees/transaction.go
📚 Learning: 2025-07-04T10:50:42.832Z
Learnt from: garry-sharp
PR: vultisig/plugin#117
File: plugin/fees/constraints.go:3-3
Timestamp: 2025-07-04T10:50:42.832Z
Learning: For the vultisig/plugin project, use "fee" (singular) throughout the codebase wherever possible, not "fees" (plural). This applies to all naming conventions including plugin types, variable names, function names, comments, and documentation.
Applied to files:
plugin/fees/transaction.go
🧬 Code graph analysis (3)
plugin/fees/helper.go (2)
plugin/fees/fees.go (1)
FeePlugin(33-48)internal/types/keysign.go (1)
PluginKeysignRequest(7-12)
plugin/fees/fees.go (1)
internal/types/keysign.go (1)
PluginKeysignRequest(7-12)
plugin/fees/transaction.go (4)
plugin/fees/fees.go (1)
FeePlugin(33-48)internal/types/fees.go (2)
FeeRunStateDraft(14-14)FeeRun(29-39)common/vault.go (1)
GetVaultFromPolicy(12-24)internal/types/keysign.go (1)
PluginKeysignRequest(7-12)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
1 participant
Summary by CodeRabbit
New Features
Behavior Changes
Refactor