Fees logic separation#145
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis change refactors the fee collection system by splitting fee handling into three asynchronous job types: loading fees, transacting fees, and post-transaction status checks. It introduces new configuration options, updates database schema and access methods, modularizes plugin logic, and adds helper utilities for Ethereum transactions. The configuration and migration files are updated to match the new workflow. Changes
Sequence Diagram(s)sequenceDiagram
participant Cron as Cron Scheduler
participant Worker as Fee Worker
participant Plugin as FeePlugin
participant DB as Database
participant Eth as Ethereum Node
participant Verifier as Verifier API
%% Loading Fees
Cron->>Worker: Trigger Load Fees Job
Worker->>Plugin: LoadFees()
Plugin->>DB: Fetch Fee Policies
Plugin->>Verifier: Get Pending Fees
Plugin->>DB: Create/Update FeeRuns
%% Transacting Fees
Cron->>Worker: Trigger Transact Fees Job
Worker->>Plugin: HandleTransactions()
Plugin->>DB: Fetch Draft FeeRuns
Plugin->>Eth: Propose & Sign Transactions
Plugin->>DB: Update FeeRun with TxHash
%% Post-Transaction Checks
Cron->>Worker: Trigger Post-Tx Job
Worker->>Plugin: HandlePostTx()
Plugin->>DB: Fetch Sent FeeRuns
Plugin->>Eth: Get Tx Receipt
Plugin->>Verifier: Mark Fee as Collected
Plugin->>DB: Update FeeRun Status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (17)
✨ 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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the fee collection system to separate the logic into distinct phases: loading fees from the verifier, executing transactions, and post-transaction status checking. The changes implement a more robust fee collection workflow with better error handling and transaction tracking.
- Splits fee collection into three separate async jobs: loading fees, handling transactions, and post-transaction processing
- Replaces transaction ID references with transaction hash strings for better blockchain integration
- Adds new database methods for comprehensive fee run management and status tracking
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| storage/postgres/policy.go | Adds method to get all fee policies and renames existing method for clarity |
| storage/postgres/migrations/plugin/20250630152230_fee_runs.sql | Updates database schema to use tx_hash instead of tx_id |
| storage/postgres/fees.go | Expands fee management with new methods for status updates and comprehensive querying |
| storage/db.go | Updates interface to reflect new fee management methods |
| service/policy.go | Updates method call to use renamed policy retrieval function |
| plugin/fees/transaction.go | Refactors transaction handling with better error handling and hash tracking |
| plugin/fees/post_tx.go | New file implementing post-transaction status checking and verification |
| plugin/fees/helper.go | New file with utility functions for transaction decoding and hash generation |
| plugin/fees/fees.go | Major refactor splitting fee collection into separate loading and transaction phases |
| plugin/fees/constraints.go | Updates task type constants for the new three-phase workflow |
| plugin/fees/config.go | Adds Ethereum provider and confirmation configuration options |
| internal/verifierapi/verifierapi.go | Adds POST request support for authenticated API calls |
| internal/verifierapi/fees.go | Implements fee collection marking functionality |
| internal/types/fees.go | Updates fee run structure to use transaction hash and adds new status types |
| etc/vultisig/fee.yml | Adds Ethereum provider configuration |
| cmd/fees/worker/main.go | Implements cron-based scheduling for the three-phase fee collection workflow |
| return fmt.Errorf("failed to decode tx: %w", err) | ||
| } | ||
|
|
||
| fp.logger.Info("33333") |
There was a problem hiding this comment.
This appears to be debug logging that should be removed from production code.
| fp.logger.Info("33333") | |
| // Removed unnecessary debug log statement. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (6)
plugin/fees/helper.go (1)
37-41: Remove unused JSON tagsThe JSON tags on this internal struct are not used anywhere in the codebase.
type erc20tx struct { - to ecommon.Address `json:"to"` - amount *big.Int `json:"amount"` - token ecommon.Address `json:"token"` + to ecommon.Address + amount *big.Int + token ecommon.Address }plugin/fees/post_tx.go (3)
33-33: Remove misleading TODO commentThe code is already implementing concurrent processing with goroutines and semaphore. The TODO comment is no longer applicable.
- //TODO go routine this sem := semaphore.NewWeighted(10)
61-63: Remove redundant nil checkThis check is already performed in
HandlePostTxbefore calling this function.- if run.TxHash == nil || run.Status == types.FeeRunStateDraft { - return nil - }
74-74: Use constant for transaction success statusReplace magic number with a named constant for better readability.
- if receipt.Status == 1 { + if receipt.Status == etypes.ReceiptStatusSuccessful {plugin/fees/fees.go (1)
249-249: Consider filtering fee runs at the database levelThe function retrieves all fee runs but then filters them in the loop. Consider passing the status filter to
GetAllFeeRunsfor better performance.- runs, err := fp.db.GetAllFeeRuns(ctx) + runs, err := fp.db.GetAllFeeRuns(ctx, types.FeeRunStateDraft)storage/postgres/fees.go (1)
176-194: Inconsistent behavior between GetFeeRuns and GetAllFeeRuns
GetFeeRunsdoesn't populate theFeesfield in the returnedFeeRunstructs, whileGetAllFeeRunsdoes. This inconsistency could confuse API users.Consider either:
- Documenting this difference clearly
- Making both methods consistent by having GetFeeRuns also load fees
- Renaming methods to clarify the difference (e.g.,
GetFeeRunsWithFeesvsGetFeeRunsWithoutFees)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
cmd/fees/worker/main.go(3 hunks)etc/vultisig/fee.yml(1 hunks)internal/types/fees.go(2 hunks)internal/verifierapi/fees.go(2 hunks)internal/verifierapi/verifierapi.go(2 hunks)plugin/fees/config.go(4 hunks)plugin/fees/constraints.go(1 hunks)plugin/fees/fees.go(7 hunks)plugin/fees/helper.go(1 hunks)plugin/fees/post_tx.go(1 hunks)plugin/fees/transaction.go(6 hunks)service/policy.go(1 hunks)storage/db.go(1 hunks)storage/postgres/fees.go(2 hunks)storage/postgres/migrations/plugin/20250630152230_fee_runs.sql(2 hunks)storage/postgres/policy.go(1 hunks)
🧰 Additional context used
🧠 Learnings (16)
📓 Common learnings
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.
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: 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.
etc/vultisig/fee.yml (4)
Learnt from: webpiratt
PR: #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.
Learnt from: garry-sharp
PR: #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: webpiratt
PR: #96
File: plugin/payroll/transaction.go:43-44
Timestamp: 2025-06-18T18:22:06.358Z
Learning: In the vultisig/plugin codebase, the hardcoded ethereumEvmChainID = big.NewInt(1) in plugin/payroll/transaction.go is intentional for the current implementation phase. The team is implementing ETH first, with plans to add other EVM chains later. The functions/methods are already designed to work with all EVM chains.
Learnt from: garry-sharp
PR: #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.
internal/verifierapi/verifierapi.go (1)
Learnt from: RaghavSood
PR: #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.
plugin/fees/constraints.go (7)
Learnt from: webpiratt
PR: #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.
Learnt from: garry-sharp
PR: #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: #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.
Learnt from: webpiratt
PR: #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.
Learnt from: webpiratt
PR: #96
File: plugin/payroll/transaction.go:510-514
Timestamp: 2025-06-18T18:20:59.510Z
Learning: The erc20ABI constant is defined in plugin/payroll/constants.go within the payroll package, making it accessible to other files in the same package like transaction.go.
Learnt from: webpiratt
PR: #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.
Learnt from: webpiratt
PR: #96
File: plugin/payroll/transaction.go:43-44
Timestamp: 2025-06-18T18:22:06.358Z
Learning: In the vultisig/plugin codebase, the hardcoded ethereumEvmChainID = big.NewInt(1) in plugin/payroll/transaction.go is intentional for the current implementation phase. The team is implementing ETH first, with plans to add other EVM chains later. The functions/methods are already designed to work with all EVM chains.
internal/types/fees.go (2)
Learnt from: webpiratt
PR: #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.
Learnt from: garry-sharp
PR: #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.
internal/verifierapi/fees.go (2)
Learnt from: garry-sharp
PR: #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: webpiratt
PR: #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.
cmd/fees/worker/main.go (2)
Learnt from: garry-sharp
PR: #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: webpiratt
PR: #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.
storage/postgres/migrations/plugin/20250630152230_fee_runs.sql (1)
Learnt from: webpiratt
PR: #89
File: storage/postgres/migrations/plugin/20250603181247_tx_indexer.sql:13-13
Timestamp: 2025-06-11T18:42:54.241Z
Learning: In the tx_indexer table (storage/postgres/migrations/plugin/*_tx_indexer.sql), the policy_id column is intentionally not defined with a foreign-key constraint to plugin_policies(id).
plugin/fees/config.go (3)
Learnt from: garry-sharp
PR: #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: webpiratt
PR: #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.
Learnt from: webpiratt
PR: #96
File: plugin/payroll/transaction.go:43-44
Timestamp: 2025-06-18T18:22:06.358Z
Learning: In the vultisig/plugin codebase, the hardcoded ethereumEvmChainID = big.NewInt(1) in plugin/payroll/transaction.go is intentional for the current implementation phase. The team is implementing ETH first, with plans to add other EVM chains later. The functions/methods are already designed to work with all EVM chains.
plugin/fees/helper.go (7)
Learnt from: garry-sharp
PR: #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: webpiratt
PR: #96
File: plugin/payroll/transaction.go:43-44
Timestamp: 2025-06-18T18:22:06.358Z
Learning: In the vultisig/plugin codebase, the hardcoded ethereumEvmChainID = big.NewInt(1) in plugin/payroll/transaction.go is intentional for the current implementation phase. The team is implementing ETH first, with plans to add other EVM chains later. The functions/methods are already designed to work with all EVM chains.
Learnt from: webpiratt
PR: #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.
Learnt from: webpiratt
PR: #125
File: plugin/payroll/transaction.go:186-197
Timestamp: 2025-07-09T22:22:36.651Z
Learning: In plugin/payroll/transaction.go, the getTokenID function is designed to return evm.ZeroAddress.Hex() when no "token" parameter constraint is found, rather than returning an error. This is expected behavior according to the team's design intent, even though the token parameter is marked as required in the recipe specification.
Learnt from: webpiratt
PR: #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.
Learnt from: webpiratt
PR: #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.
Learnt from: webpiratt
PR: #96
File: plugin/payroll/transaction.go:510-514
Timestamp: 2025-06-18T18:20:59.510Z
Learning: The erc20ABI constant is defined in plugin/payroll/constants.go within the payroll package, making it accessible to other files in the same package like transaction.go.
storage/postgres/policy.go (4)
Learnt from: webpiratt
PR: #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.
Learnt from: garry-sharp
PR: #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: #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.
Learnt from: webpiratt
PR: #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.
plugin/fees/post_tx.go (2)
Learnt from: webpiratt
PR: #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.
Learnt from: garry-sharp
PR: #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.
storage/db.go (6)
Learnt from: garry-sharp
PR: #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: webpiratt
PR: #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.
Learnt from: RaghavSood
PR: #75
File: storage/postgres/db_plugin.go:25-40
Timestamp: 2025-05-30T02:44:31.711Z
Learning: The embed.FS directive //go:embed migrations/plugin/*.sql in storage/postgres/db_plugin.go correctly embeds plugin migration files that exist in the storage/postgres/migrations/plugin/ directory. The path "migrations/plugin" passed to goose.Up() references the embedded filesystem structure, not the physical file system path.
Learnt from: garry-sharp
PR: #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.
Learnt from: webpiratt
PR: #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.
Learnt from: RaghavSood
PR: #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.
plugin/fees/fees.go (9)
Learnt from: garry-sharp
PR: #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: webpiratt
PR: #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.
Learnt from: garry-sharp
PR: #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.
Learnt from: webpiratt
PR: #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.
Learnt from: webpiratt
PR: #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.
Learnt from: RaghavSood
PR: #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.
Learnt from: johnnyluo
PR: #108
File: cmd/payroll/worker/main.go:84-84
Timestamp: 2025-07-02T04:58:30.139Z
Learning: VaultServiceConfig is defined as a field of type vault_config.Config from the external package "github.com/vultisig/verifier/vault_config" in worker configuration structs across the vultisig/plugin codebase (cmd/payroll/worker/config.go, cmd/fees/worker/config.go, cmd/dca/worker/config.go). The vault_config.Config struct contains an EncryptionSecret field that can be accessed via cfg.VaultServiceConfig.EncryptionSecret.
Learnt from: johnnyluo
PR: #108
File: Dockerfile.Payroll.server:14-19
Timestamp: 2025-07-02T04:55:36.331Z
Learning: In the vultisig/plugin repository, the team maintains both the main repository and the go-wrappers dependency repository, so they are comfortable downloading from the master branch rather than pinning to specific commits.
Learnt from: webpiratt
PR: #96
File: plugin/payroll/transaction.go:43-44
Timestamp: 2025-06-18T18:22:06.358Z
Learning: In the vultisig/plugin codebase, the hardcoded ethereumEvmChainID = big.NewInt(1) in plugin/payroll/transaction.go is intentional for the current implementation phase. The team is implementing ETH first, with plans to add other EVM chains later. The functions/methods are already designed to work with all EVM chains.
plugin/fees/transaction.go (11)
Learnt from: webpiratt
PR: #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.
Learnt from: garry-sharp
PR: #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: webpiratt
PR: #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.
Learnt from: webpiratt
PR: #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.
Learnt from: garry-sharp
PR: #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.
Learnt from: RaghavSood
PR: #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.
Learnt from: webpiratt
PR: #105
File: plugin/dca/dca.go:734-736
Timestamp: 2025-07-01T17:35:35.277Z
Learning: The DCA plugin in the vultisig/plugin codebase is currently not fully implemented. Methods like getCompletedSwapTransactionsCount() return placeholder values (e.g., 0) with TODO comments, which is expected behavior during the development phase. The plugin implementation is intentionally incomplete while other parts of the system are being refactored.
Learnt from: webpiratt
PR: #125
File: plugin/payroll/policy.go:160-160
Timestamp: 2025-07-09T22:23:17.348Z
Learning: In plugin/payroll/policy.go, the "token" case in the checkRule method intentionally does not call validateToken() as this is a temporary implementation that will be replaced with generic schema validation. The missing validation call is expected behavior during this transition period.
Learnt from: webpiratt
PR: #96
File: plugin/payroll/transaction.go:43-44
Timestamp: 2025-06-18T18:22:06.358Z
Learning: In the vultisig/plugin codebase, the hardcoded ethereumEvmChainID = big.NewInt(1) in plugin/payroll/transaction.go is intentional for the current implementation phase. The team is implementing ETH first, with plans to add other EVM chains later. The functions/methods are already designed to work with all EVM chains.
Learnt from: webpiratt
PR: #125
File: plugin/payroll/transaction.go:186-197
Timestamp: 2025-07-09T22:22:36.651Z
Learning: In plugin/payroll/transaction.go, the getTokenID function is designed to return evm.ZeroAddress.Hex() when no "token" parameter constraint is found, rather than returning an error. This is expected behavior according to the team's design intent, even though the token parameter is marked as required in the recipe specification.
Learnt from: webpiratt
PR: #127
File: internal/keysign/signer.go:134-142
Timestamp: 2025-07-19T12:28:15.328Z
Learning: In internal/keysign/signer.go, the continue statement in the CheckKeysignComplete error handling (around line 140) is intentional. The code should attempt to check all messages in each polling iteration, even if some CheckKeysignComplete calls fail, before proceeding to the next polling cycle. This provides resilience for temporary failures on individual messages.
storage/postgres/fees.go (5)
Learnt from: garry-sharp
PR: #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: webpiratt
PR: #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.
Learnt from: webpiratt
PR: #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.
Learnt from: RaghavSood
PR: #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.
Learnt from: garry-sharp
PR: #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.
🧬 Code Graph Analysis (5)
internal/verifierapi/fees.go (1)
internal/verifierapi/verifierapi.go (1)
VerifierApi(26-31)
cmd/fees/worker/main.go (1)
plugin/fees/constraints.go (3)
TypeFeeLoad(6-6)TypeFeeTransact(7-7)TypeFeePostTx(8-8)
storage/postgres/policy.go (1)
storage/postgres/db.go (1)
PostgresBackend(15-17)
plugin/fees/fees.go (1)
internal/types/fees.go (2)
FeeRunStateDraft(14-14)FeeRun(29-39)
storage/postgres/fees.go (3)
storage/postgres/db.go (1)
PostgresBackend(15-17)internal/types/fees.go (5)
FeeRunState(11-11)FeeRun(29-39)FeeRunStateSent(15-15)FeeRunStateSuccess(16-16)Fee(21-26)internal/verifierapi/fees.go (1)
FeeDto(12-21)
⏰ 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 (30)
service/policy.go (1)
133-133: LGTM! Method call updated to align with database layer refactoring.The change from
GetAllPluginPoliciestoGetPluginPoliciescorrectly reflects the database layer refactoring while maintaining the same interface contract.internal/verifierapi/fees.go (2)
7-7: LGTM! Time import correctly added for the new method.The time package import is properly added to support the
time.Timeparameter in the newMarkFeeAsCollectedmethod.
60-84: LGTM! Well-implemented method with proper error handling.The
MarkFeeAsCollectedmethod follows established patterns in the codebase:
- Clear parameter types and purpose
- Proper JSON request body structure
- Comprehensive error handling with descriptive messages
- Correct HTTP status code validation
- Proper resource cleanup with deferred Body.Close()
storage/postgres/migrations/plugin/20250630152230_fee_runs.sql (2)
9-9: LGTM! Correct column type for Ethereum transaction hashes.VARCHAR(66) is the appropriate size for Ethereum transaction hashes (2 characters for "0x" prefix + 64 hex characters).
28-28: LGTM! View correctly updated to reflect schema changes.The
fee_run_with_totalsview is properly updated to selecttx_hashinstead oftx_idand includes it in the GROUP BY clause, maintaining consistency with the table schema changes.Also applies to: 34-34
internal/types/fees.go (3)
14-18: LGTM! Well-structured state constants.The addition of
FeeRunStateSuccessandFeeRunStateFailedconstants provides clear completion states for the fee processing workflow. The naming follows the established "fee" singular convention.
34-34: LGTM! Transaction hash as string is more appropriate.Changing from
TxID *uuid.UUIDtoTxHash *stringis the correct approach for storing Ethereum transaction hashes, which are hex strings, not UUIDs.
38-38: LGTM! Embedded fees provide convenient access.Adding the
Fees []Feeslice allows direct access to individual fees within a fee run, supporting the modular processing workflow described in the PR objectives.plugin/fees/config.go (4)
41-42: LGTM! Configuration supports new transaction confirmation workflow.The addition of
EthProviderandSuccessConfirmationsfields properly extends the configuration to support the post-transaction confirmation logic described in the PR objectives.
55-55: LGTM! Reasonable default for confirmation threshold.Setting the default
SuccessConfirmationsto 20 is appropriate for Ethereum mainnet, providing a good balance between security and processing time.
93-105: LGTM! Configuration option functions follow established patterns.The
WithEthClientandWithSuccessConfirmationsfunctions maintain consistency with existing configuration option patterns in the codebase.
175-177: LGTM! Proper validation for required configuration.Adding validation for the
EthProviderfield ensures the configuration is complete before the plugin operates, preventing runtime failures.cmd/fees/worker/main.go (5)
25-40: LGTM! Well-structured fee loading orchestration.The
startLoadingFeesfunction properly marshals the fee collection configuration and enqueues the task with appropriate retry and timeout settings.
42-51: LGTM! Clean transaction orchestration.The
startTransactingFeesfunction correctly enqueues the transaction task with empty payload, as the handler will determine which fee runs to process.
53-62: LGTM! Appropriate post-transaction monitoring.The
startPostTxfunction properly enqueues the status checking task to monitor transaction confirmations.
176-178: LGTM! Task handlers aligned with new workflow phases.The three separate task handlers (
TypeFeeLoad,TypeFeeTransact,TypeFeePostTx) properly implement the separated fee processing workflow described in the PR objectives.
180-199: LGTM! Well-designed cron schedules for different phases.The cron schedules are appropriately configured:
- Loading fees every 10 minutes ensures regular fee discovery
- Transacting fees weekly on Fridays provides batch processing
- Post-transaction checks every 5 minutes enables timely confirmation monitoring
storage/postgres/policy.go (2)
41-71: LGTM! Well-designed fee-specific policy retrieval.The
GetAllFeePoliciesmethod properly implements fee-specific policy retrieval with:
- Correct filtering for the fee plugin ID 'vultisig-fees-feee'
DISTINCT ON(public_key)withORDER BY created_at DESCto get the latest policy per public key- Proper error handling and row scanning
73-114: LGTM! Preserved functionality with proper validation.The
GetPluginPoliciesmethod maintains the original functionality while adding the essential nil pool check that prevents runtime panics.plugin/fees/helper.go (2)
18-35: LGTM!The
getHashfunction correctly reconstructs a signed Ethereum transaction from its components.
110-115: LGTM!The utility function correctly handles hex strings with or without the "0x" prefix.
storage/db.go (6)
19-19: Method renamed appropriately for clarity.The renaming from
GetAllPluginPoliciestoGetPluginPoliciesbetter reflects that this method can filter policies by parameters rather than always returning all policies.
20-20: New method follows naming convention correctly.The
GetAllFeePoliciesmethod name correctly uses "Fee" (singular) as per the established naming convention for this codebase.
25-25: Variadic parameter change improves API usability.The change from
fees []verifierapi.FeeDtotofees ...verifierapi.FeeDtomakes the method more convenient to call with individual fee arguments while maintaining backward compatibility.
27-32: New fee run management methods provide comprehensive CRUD operations.The newly added methods create a complete interface for fee run lifecycle management:
SetFeeRunSuccess: Updates run status to successfulGetAllFeeRuns: Retrieves runs with optional status filteringGetFees: Fetches fees by IDsGetPendingFeeRun: Gets pending run for a policyCreateFee: Creates individual fee entriesGetFeeRuns: Gets runs by specific stateThe naming consistently uses "Fee" (singular) following the established convention.
28-28: GetAllFeeRuns variadic handling is correct
The implementation checkslen(statuses) == 0and issues an unfiltered query when no statuses are passed, matching the documented behavior.
26-26: Verified SetFeeRunSent call sites updatedAll usages of
SetFeeRunSentnow pass a string-based transaction hash. In particular:
- Interface and implementation in
storage/db.gostorage/postgres/fees.go
both defineSetFeeRunSent(ctx context.Context, runId uuid.UUID, txHash string) error.- The sole call in
plugin/fees/transaction.gonow correctly usestx.Hash().Hex()(string) when invokingfp.db.SetFeeRunSent.No callers remain passing a UUID for the transaction identifier.
plugin/fees/fees.go (1)
95-98: LGTM! Proper Ethereum client initializationThe Ethereum client is correctly initialized with proper error handling.
plugin/fees/transaction.go (2)
158-160: No external references to FeePlugin.ProposeTransactions; stub is safe
Ran a search forProposeTransactions(outside ofplugin/feesand only found calls to the local implementations in:
- plugin/dca/dca.go (DCAPlugin.ProposeTransactions)
- plugin/payroll/transaction.go (Plugin.ProposeTransactions)
There are no invocations of FeePlugin.ProposeTransactions elsewhere, so the “not implemented” stub won’t break external callers.
279-281: No external dependencies call FeePlugin.SigningCompleteA grep search (
rg -A 3 'SigningComplete\(' --glob '!plugin/fees/*') found no references to the stub inplugin/fees/transaction.gooutside its own package. TheSigningCompletemethod in the fees plugin isn’t invoked elsewhere in this repo.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
plugin/fees/fees.go (3)
143-145: Use proper context and structured logging.The semaphore acquisition should use the passed context instead of
context.Background(), and structured logging should replace error handling patterns.- if err := sem.Acquire(context.Background(), 1); err != nil { - return fmt.Errorf("failed to acquire semaphore: %w", err) + if err := sem.Acquire(ctx, 1); err != nil { + fp.logger.WithError(err).Error("Failed to acquire semaphore") + return fmt.Errorf("failed to acquire semaphore: %w", err)
224-224: Handle CreateFee error properly.The error returned by CreateFee is not being handled, which could lead to silent failures.
- fp.db.CreateFee(ctx, run.ID, fee) + if err := fp.db.CreateFee(ctx, run.ID, fee); err != nil { + return fmt.Errorf("failed to create fee: %w", err) + }
287-289: Use proper context instead of Background.Same issue with semaphore acquisition using context.Background() instead of the passed context.
- if err := sem.Acquire(context.Background(), 1); err != nil { - return fmt.Errorf("failed to acquire semaphore: %w", err) + if err := sem.Acquire(ctx, 1); err != nil { + fp.logger.WithError(err).Error("Failed to acquire semaphore") + return fmt.Errorf("failed to acquire semaphore: %w", err)
🧹 Nitpick comments (1)
etc/vultisig/fee.yml (1)
10-10: Add newline at end of file.The file is missing a newline character at the end, which violates YAML formatting standards.
-eth_provider: https://rpc.ankr.com/eth +eth_provider: https://rpc.ankr.com/eth +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
cmd/fees/worker/main.go(3 hunks)etc/vultisig/fee.yml(1 hunks)internal/types/fees.go(2 hunks)internal/verifierapi/fees.go(2 hunks)internal/verifierapi/verifierapi.go(2 hunks)plugin/fees/config.go(4 hunks)plugin/fees/constraints.go(1 hunks)plugin/fees/fees.go(7 hunks)plugin/fees/helper.go(1 hunks)plugin/fees/post_tx.go(1 hunks)plugin/fees/transaction.go(6 hunks)service/policy.go(1 hunks)storage/db.go(1 hunks)storage/postgres/fees.go(2 hunks)storage/postgres/migrations/plugin/20250630152230_fee_runs.sql(2 hunks)storage/postgres/policy.go(1 hunks)storage/postgres/schema/schema.sql(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- service/policy.go
- internal/verifierapi/fees.go
- storage/postgres/migrations/plugin/20250630152230_fee_runs.sql
- plugin/fees/constraints.go
- internal/verifierapi/verifierapi.go
- plugin/fees/config.go
- internal/types/fees.go
- plugin/fees/post_tx.go
- cmd/fees/worker/main.go
- plugin/fees/helper.go
- storage/postgres/policy.go
- storage/postgres/fees.go
- storage/db.go
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
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.
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: 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.
storage/postgres/schema/schema.sql (2)
Learnt from: webpiratt
PR: #89
File: storage/postgres/migrations/plugin/20250603181247_tx_indexer.sql:13-13
Timestamp: 2025-06-11T18:42:54.241Z
Learning: In the tx_indexer table (storage/postgres/migrations/plugin/*_tx_indexer.sql), the policy_id column is intentionally not defined with a foreign-key constraint to plugin_policies(id).
Learnt from: garry-sharp
PR: #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.
plugin/fees/fees.go (8)
Learnt from: garry-sharp
PR: #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: webpiratt
PR: #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.
Learnt from: garry-sharp
PR: #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.
Learnt from: webpiratt
PR: #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.
Learnt from: webpiratt
PR: #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.
Learnt from: RaghavSood
PR: #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.
Learnt from: johnnyluo
PR: #108
File: cmd/payroll/worker/main.go:84-84
Timestamp: 2025-07-02T04:58:30.139Z
Learning: VaultServiceConfig is defined as a field of type vault_config.Config from the external package "github.com/vultisig/verifier/vault_config" in worker configuration structs across the vultisig/plugin codebase (cmd/payroll/worker/config.go, cmd/fees/worker/config.go, cmd/dca/worker/config.go). The vault_config.Config struct contains an EncryptionSecret field that can be accessed via cfg.VaultServiceConfig.EncryptionSecret.
Learnt from: webpiratt
PR: #96
File: plugin/payroll/transaction.go:43-44
Timestamp: 2025-06-18T18:22:06.358Z
Learning: In the vultisig/plugin codebase, the hardcoded ethereumEvmChainID = big.NewInt(1) in plugin/payroll/transaction.go is intentional for the current implementation phase. The team is implementing ETH first, with plans to add other EVM chains later. The functions/methods are already designed to work with all EVM chains.
plugin/fees/transaction.go (10)
Learnt from: webpiratt
PR: #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.
Learnt from: garry-sharp
PR: #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: webpiratt
PR: #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.
Learnt from: webpiratt
PR: #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.
Learnt from: garry-sharp
PR: #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.
Learnt from: RaghavSood
PR: #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.
Learnt from: webpiratt
PR: #125
File: plugin/payroll/policy.go:160-160
Timestamp: 2025-07-09T22:23:17.348Z
Learning: In plugin/payroll/policy.go, the "token" case in the checkRule method intentionally does not call validateToken() as this is a temporary implementation that will be replaced with generic schema validation. The missing validation call is expected behavior during this transition period.
Learnt from: webpiratt
PR: #105
File: plugin/dca/dca.go:734-736
Timestamp: 2025-07-01T17:35:35.277Z
Learning: The DCA plugin in the vultisig/plugin codebase is currently not fully implemented. Methods like getCompletedSwapTransactionsCount() return placeholder values (e.g., 0) with TODO comments, which is expected behavior during the development phase. The plugin implementation is intentionally incomplete while other parts of the system are being refactored.
Learnt from: webpiratt
PR: #125
File: plugin/payroll/transaction.go:186-197
Timestamp: 2025-07-09T22:22:36.651Z
Learning: In plugin/payroll/transaction.go, the getTokenID function is designed to return evm.ZeroAddress.Hex() when no "token" parameter constraint is found, rather than returning an error. This is expected behavior according to the team's design intent, even though the token parameter is marked as required in the recipe specification.
Learnt from: webpiratt
PR: #127
File: internal/keysign/signer.go:134-142
Timestamp: 2025-07-19T12:28:15.328Z
Learning: In internal/keysign/signer.go, the continue statement in the CheckKeysignComplete error handling (around line 140) is intentional. The code should attempt to check all messages in each polling iteration, even if some CheckKeysignComplete calls fail, before proceeding to the next polling cycle. This provides resilience for temporary failures on individual messages.
etc/vultisig/fee.yml (5)
Learnt from: webpiratt
PR: #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.
Learnt from: garry-sharp
PR: #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: webpiratt
PR: #96
File: plugin/payroll/transaction.go:43-44
Timestamp: 2025-06-18T18:22:06.358Z
Learning: In the vultisig/plugin codebase, the hardcoded ethereumEvmChainID = big.NewInt(1) in plugin/payroll/transaction.go is intentional for the current implementation phase. The team is implementing ETH first, with plans to add other EVM chains later. The functions/methods are already designed to work with all EVM chains.
Learnt from: webpiratt
PR: #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.
Learnt from: garry-sharp
PR: #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.
🪛 YAMLlint (1.37.1)
etc/vultisig/fee.yml
[error] 10-10: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (14)
storage/postgres/schema/schema.sql (2)
74-74: LGTM! Well-designed schema change for transaction hash tracking.The replacement of
tx_id(UUID) withtx_hash(varchar(66)) properly accommodates Ethereum transaction hashes including the "0x" prefix. The length of 66 characters is correct for this purpose.
84-90: View update is consistent with schema change.The fee_run_with_totals view correctly uses
tx_hashinstead oftx_idin both the SELECT and GROUP BY clauses, maintaining consistency with the table schema changes.plugin/fees/fees.go (5)
6-6: LGTM! Good additions for concurrency control.The sync and semaphore imports support the new concurrency control mechanisms introduced in the refactoring.
Also applies to: 18-18
49-51: Well-designed synchronization and Ethereum client integration.The transactingMutex provides proper synchronization between fee loading and transaction handling phases, and the ethClient field enables direct Ethereum interaction.
96-100: Proper Ethereum client initialization.The ethClient initialization using the EthProvider configuration is correctly implemented with appropriate error handling.
122-156: Excellent separation of concerns in LoadFees method.The LoadFees method properly separates fee loading from transaction handling, uses appropriate concurrency controls with semaphores, and implements good error handling patterns. The mutex lock ensures no conflicts with transaction processing.
242-306: Well-structured HandleTransactions method with good concurrency control.The HandleTransactions method properly coordinates transaction processing with mutex synchronization, appropriate filtering of fee runs, and concurrent execution with semaphore limits.
plugin/fees/transaction.go (7)
7-7: Good additions for error handling and type definitions.The errors import and internal types import support the refactored transaction handling logic.
Also applies to: 30-30
33-156: Well-implemented proposeTransactions method.The new internal proposeTransactions method properly generates unsigned ERC20 transfer transactions based on fee run data. The logic correctly validates policy-run matching, extracts recipient and token constraints, and constructs the transaction with appropriate error handling.
158-160: Appropriate stub for deprecated method.The ProposeTransactions method is correctly stubbed as the functionality has been moved to the internal proposeTransactions method used by the new workflow.
181-228: Excellent refactoring of initSign method.The initSign method now handles the complete transaction lifecycle including decoding, signing, broadcasting via Ethereum client, and status updates. The comprehensive logging and proper error handling make this a robust implementation.
187-194: Good error aggregation pattern.The use of
errors.Jointo aggregate multiple potential decoding errors is a clean approach to error handling.
206-212: Comprehensive transaction logging.The structured logging with transaction details (hash, recipient, amount, token) provides excellent visibility for debugging and monitoring fee collection transactions.
277-279: Appropriate stub for deprecated SigningComplete.The SigningComplete method is correctly stubbed as its functionality has been integrated into the initSign method in the new workflow.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
fees.md (3)
1-5: Use singular “fee” to stay consistent with project-wide conventionPer long-term learning, the vultisig/plugin codebase (code and docs) should consistently use “fee” (singular). The title and the opening sentence still use “Fees”.
Proposed tweak:
-# Fees System v2 - Comprehensive Analysis +# Fee System v2 - Comprehensive Analysis ... -This document provides a comprehensive analysis of the fees system implementation in the fees-v2 branch, +This document provides a comprehensive analysis of the fee system implementation in the fee-v2 branch,
11-18: Function names in snippet violate the singular “fee” guidelineThe example shows
LoadFees,HandleTransactions,HandlePostTx. Consider aligning the snippet (and eventually the code) with the singular convention to avoid confusion.-LoadFees() // Phase 1: Load fees from verifier into database -HandleTransactions() // Phase 2: Process fee runs and create transactions -HandlePostTx() // Phase 3: Monitor and finalize transactions +LoadFee() // Phase 1: Load fee data from verifier into database +HandleTransaction() // Phase 2: Process fee runs and create transactions +HandlePostTx() // Phase 3: Monitor and finalize transactionsIf the underlying functions remain plural for external-API compatibility, add a brief note explaining the exception.
127-133: Hard-coded semaphore weight should be documented as temporary or made configurableThe doc already flags the magic number
10, but consider inserting a TODO block or config excerpt showing the intended parameter, so readers don’t assume the constant is final.-sem := semaphore.NewWeighted(10) -// Why 10? Should be configurable +// Concurrency limit – TODO: move to config (`fee.max_concurrent_tx_process`) +sem := semaphore.NewWeighted(fp.config.MaxConcurrentTxProcess)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
fees.md(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
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.
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: 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: for the vultisig/plugin project, use "fee" (singular) throughout the codebase wherever possible, not...
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:
fees.md
📚 Learning: for the fee plugin in plugin/fees/policy.go, the resource validation and recipe specification are in...
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:
fees.md
📚 Learning: the constant `vtypes.pluginvultisigfees_feee` in the vultisig/plugin project is correctly spelled wi...
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:
fees.md
📚 Learning: in the vultisig/plugin codebase, duplicate transaction prevention for the payroll plugin has been mo...
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:
fees.md
|
Going to reopen elsewhere with cleaner history |
Summary by CodeRabbit
New Features
Configuration
Bug Fixes
Database & Migration
Refactor