🚧 fees v2#146
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. WalkthroughSplits the fees workflow into three scheduled stages (load, transact, post-tx); adds cron-driven enqueuers and Asynq handlers; refactors plugin into per-policy loading, per-run transacting and post‑tx verification with Ethereum helpers; replaces TxID UUID with TxHash string across types, DB, and migrations; adds verifier POST API for marking collected fees. Changes
Sequence Diagram(s)sequenceDiagram
participant Cron as CronScheduler
participant AQ as AsynqClient
participant W as Worker
participant FP as FeePlugin
participant DB as Database
participant VA as VerifierAPI
participant ETH as Ethereum
Cron->>AQ: Enqueue fees:load
AQ->>W: Deliver fees:load
W->>FP: LoadFees()
FP->>DB: GetAllFeePolicies / CreateFeeRun / CreateFee
Cron->>AQ: Enqueue fees:transaction
AQ->>W: Deliver fees:transaction
W->>FP: HandleTransactions()
FP->>DB: GetFeeRuns(state=Draft)
FP->>ETH: proposeTransactions → sign & send
FP->>DB: SetFeeRunSent(tx_hash)
Cron->>AQ: Enqueue fees:post_tx
AQ->>W: Deliver fees:post_tx
W->>FP: HandlePostTx()
FP->>DB: GetAllFeeRuns(status=Sent)
FP->>ETH: GetTransactionReceipt(tx_hash)
alt confirmed & success
FP->>VA: MarkFeeAsCollected(tx_hash, time, feeIds)
FP->>DB: SetFeeRunSuccess(runId)
else failed / insufficient confirmations
FP->>DB: (no change / mark failed)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the "Integrations" page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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 implements version 2 of the fees plugin, restructuring the fee collection system from a single-step process to a multi-stage pipeline with improved transaction tracking and status management.
- Replaces UUID-based transaction references with string-based transaction hashes
- Introduces a three-phase job system: fee loading, transaction processing, and post-transaction status checking
- Adds comprehensive fee run lifecycle management with proper state transitions
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| storage/postgres/schema/schema.sql | Updates fee_run table to use tx_hash instead of tx_id and removes foreign key constraint |
| storage/postgres/migrations/plugin/20250630152230_fee_runs.sql | Migration reflecting the tx_hash schema change |
| storage/postgres/fees.go | Adds new fee management methods and updates existing ones to use tx_hash |
| storage/postgres/policy.go | Adds GetAllFeePolicies method and renames existing method |
| storage/db.go | Updates interface to include new fee management methods |
| service/policy.go | Updates method call to renamed function |
| plugin/fees/transaction.go | Refactors transaction handling with proper hash generation and status updates |
| plugin/fees/post_tx.go | New file implementing post-transaction status checking logic |
| plugin/fees/helper.go | New utility functions for transaction decoding and hash generation |
| plugin/fees/fees.go | Major restructure splitting fee collection into load/transaction phases |
| plugin/fees/constraints.go | Updates task type constants for the new three-phase system |
| plugin/fees/config.go | Restructures configuration for job-based processing |
| internal/verifierapi/verifierapi.go | Adds POST request support for API calls |
| internal/verifierapi/fees.go | Adds method to mark fees as collected |
| internal/types/fees.go | Updates FeeRun type to use tx_hash and adds new states |
| etc/vultisig/fee.yml | Updates configuration file structure |
| cmd/fees/worker/main.go | Implements cron-based job scheduling for the three phases |
Comments suppressed due to low confidence (1)
plugin/fees/config.go:19
- [nitpick] The field name 'chainId' uses camelCase while 'ChainId' uses PascalCase. Consider using consistent naming convention, preferably 'chainID' and 'ChainID' to follow Go naming conventions for abbreviations.
chainId uint64 `mapstructure:"chain_id"` // The chain ID of the Ethereum blockchain.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🔭 Outside diff range comments (1)
storage/postgres/fees.go (1)
24-26: Use constant instead of hardcoded string for plugin IDConsider importing and using the
vtypes.PluginVultisigFees_feeeconstant instead of the hardcoded string to maintain consistency and avoid typos.Import the types package at the top:
import vtypes "github.com/vultisig/verifier/types"Then update the comparison:
- if pluginId != "vultisig-fees-feee" { - return nil, errors.New("plugin id not found or not vultisig-fees-feee") + if pluginId != string(vtypes.PluginVultisigFees_feee) { + return nil, errors.New("plugin id not found or not " + string(vtypes.PluginVultisigFees_feee))
🧹 Nitpick comments (12)
etc/vultisig/fee.yml (1)
7-16: Excellent job scheduling configuration with appropriate timing.The three-stage job configuration supports the new fee processing workflow effectively:
load(@every 2m): Frequent fee discoverytransact(0 12 * * 5): Weekly execution on Fridays at noon - appropriate for batch processingpost(@every 5m): Regular post-transaction status updatesThe
max_concurrent_jobs: 10setting provides good concurrency control.Fix the missing newline at the end of the file:
post: cronexpr: "@every 5m" - max_concurrent_jobs: 10 + max_concurrent_jobs: 10 +plugin/fees/constraints.go (1)
6-8: Consider using singular "fee" for consistency.Based on the established naming convention for this project, consider using "fee:" instead of "fees:" prefix for these constants to maintain consistency with the singular form used throughout the codebase.
-const TypeFeeLoad = "fees:load" // Load list of pending fees into the db from the verifier -const TypeFeeTransact = "fees:transaction" // Collect a list of loaded fees from the users wallet -const TypeFeePostTx = "fees:post_tx" // Check the status of the fee runs +const TypeFeeLoad = "fee:load" // Load list of pending fees into the db from the verifier +const TypeFeeTransact = "fee:transaction" // Collect a list of loaded fees from the users wallet +const TypeFeePostTx = "fee:post_tx" // Check the status of the fee runsplugin/fees/post_tx.go (3)
60-63: Remove redundant validationThese checks are already performed by the caller in
HandlePostTx(lines 46-48), making them redundant here.func (fp *FeePlugin) updateStatus(ctx context.Context, run types.FeeRun, currentBlock uint64) error { - if run.TxHash == nil || run.Status == types.FeeRunStateDraft { - return nil - } fp.logger.WithFields(logrus.Fields{"run_id": run.ID}).Info("Beginning status check/update")
77-80: Pre-allocate slice for better performancePre-allocating the slice with known capacity avoids multiple allocations during append operations.
- ids := []uuid.UUID{} + ids := make([]uuid.UUID, 0, len(run.Fees)) for _, fee := range run.Fees { ids = append(ids, fee.ID) }
95-97: Implement failed transaction handlingThe TODO comment indicates missing critical logic for handling failed transactions. This should be implemented to ensure proper state management.
Would you like me to help implement the failed transaction logic or create an issue to track this task?
plugin/fees/helper.go (2)
75-75: Move ABI definition to constantsThe transfer ABI string should be defined as a package-level constant for better maintainability and reusability.
Add at the package level:
const erc20TransferABI = `[{"name":"transfer","type":"function","inputs":[{"name":"to","type":"address"},{"name":"value","type":"uint256"}],"outputs":[{"name":"","type":"bool"}]}]`Then use it:
- const transferABI = `[{"name":"transfer","type":"function","inputs":[{"name":"to","type":"address"},{"name":"value","type":"uint256"}],"outputs":[{"name":"","type":"bool"}]}]` - parsedABI, err := abi.JSON(strings.NewReader(transferABI)) + parsedABI, err := abi.JSON(strings.NewReader(erc20TransferABI))
78-78: Improve error messages for better debuggingThe error messages are too generic and don't provide enough context for debugging.
- return nil, fmt.Errorf("failed to parse ABI") + return nil, fmt.Errorf("failed to parse ERC20 transfer ABI: %w", err) - return nil, fmt.Errorf("unknown method ID") + return nil, fmt.Errorf("unknown method ID: expected ERC20 transfer, got %x", tx.Data[:4]) - return nil, fmt.Errorf("failed get recipient and amount from tx") + return nil, fmt.Errorf("failed to unpack recipient and amount from tx data: %w", err)Also applies to: 84-84, 90-90
plugin/fees/fees.go (1)
191-202: Optimize fee existence checkInstead of retrieving all fee data just to check existence, consider adding a more efficient method like
FeeExists(ctx, feeID)to the database interface.This would improve performance by avoiding unnecessary data retrieval when you only need to check existence.
plugin/fees/transaction.go (1)
220-223: Add transaction state recovery mechanismThe comment indicates this is critical code where errors could cause state mismatches. Consider implementing a recovery mechanism or two-phase commit pattern.
Consider:
- Using database transactions to ensure atomicity
- Implementing a recovery mechanism to handle partial failures
- Adding monitoring/alerting for state mismatches
plugin/fees/config.go (2)
24-24: Fix comment accuracyThe comments mention "consecutive tasks" but the fields control concurrent tasks.
- MaxConcurrentJobs uint64 `mapstructure:"max_concurrent_jobs"` //How many consecutive tasks can take place + MaxConcurrentJobs uint64 `mapstructure:"max_concurrent_jobs"` // Maximum number of concurrent tasksAlso applies to: 32-32
19-20: Consider consistent chain ID handlingHaving both
chainId(unexported) andChainId(exported) fields could be confusing. Consider using only the exported field with custom unmarshaling.Implement a custom UnmarshalJSON or use a setter method to handle the conversion directly during configuration loading.
Also applies to: 129-129
storage/postgres/fees.go (1)
176-194: Consider consolidating with GetAllFeeRunsThis method provides similar functionality to
GetAllFeeRunsbut without fetching associated fees. If the intent is performance optimization by not loading fees, this is fine. Otherwise, consider usingGetAllFeeRunswith a single status parameter.
📜 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(8 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)
🧰 Additional context used
🧠 Learnings (15)
📓 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: 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.
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 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:
plugin/fees/config.gostorage/db.goplugin/fees/fees.goplugin/fees/constraints.goplugin/fees/helper.gocmd/fees/worker/main.gostorage/postgres/schema/schema.sqlinternal/verifierapi/fees.goetc/vultisig/fee.ymlstorage/postgres/policy.goplugin/fees/post_tx.gointernal/types/fees.goplugin/fees/transaction.gostorage/postgres/fees.go
📚 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:
plugin/fees/config.gostorage/db.goplugin/fees/fees.goplugin/fees/constraints.goplugin/fees/helper.gocmd/fees/worker/main.gointernal/verifierapi/fees.goetc/vultisig/fee.ymlstorage/postgres/policy.goplugin/fees/post_tx.goplugin/fees/transaction.gostorage/postgres/fees.go
📚 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:
plugin/fees/config.gostorage/db.goplugin/fees/fees.goplugin/fees/constraints.goplugin/fees/helper.gointernal/verifierapi/fees.goetc/vultisig/fee.ymlstorage/postgres/policy.gointernal/types/fees.goplugin/fees/transaction.gostorage/postgres/fees.go
📚 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:
plugin/fees/config.gostorage/db.goplugin/fees/fees.goplugin/fees/constraints.gostorage/postgres/policy.goplugin/fees/transaction.gostorage/postgres/fees.go
📚 Learning: vaultserviceconfig is defined as a field of type vault_config.config from the external package "gith...
Learnt from: johnnyluo
PR: vultisig/plugin#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.
Applied to files:
plugin/fees/config.goplugin/fees/fees.go
📚 Learning: in the vultisig/plugin codebase, the hardcoded ethereumevmchainid = big.newint(1) in plugin/payroll/...
Learnt from: webpiratt
PR: vultisig/plugin#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.
Applied to files:
plugin/fees/config.goplugin/fees/fees.goplugin/fees/constraints.goplugin/fees/helper.goetc/vultisig/fee.yml
📚 Learning: vaultserviceconfig is defined as a field of type vault_config.config in worker configuration structs...
Learnt from: johnnyluo
PR: vultisig/plugin#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 in worker configuration structs across the vultisig/plugin codebase (cmd/payroll/worker/config.go, cmd/fees/worker/config.go, cmd/dca/worker/config.go). It contains an EncryptionSecret field that can be accessed via cfg.VaultServiceConfig.EncryptionSecret.
Applied to files:
plugin/fees/config.go
📚 Learning: the embed.fs directive `//go:embed migrations/plugin/*.sql` in storage/postgres/db_plugin.go correct...
Learnt from: RaghavSood
PR: vultisig/plugin#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.
Applied to files:
storage/db.go
📚 Learning: in the payroll plugin (plugin/payroll/transaction.go), signrequest.transaction is stored with the "0...
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.goplugin/fees/constraints.goplugin/fees/helper.goplugin/fees/transaction.go
📚 Learning: the import path `github.com/vultisig/verifier/plugin` refers to an external dependency that provides...
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.gointernal/verifierapi/verifierapi.goplugin/fees/transaction.gostorage/postgres/fees.go
📚 Learning: the erc20abi constant is defined in plugin/payroll/constants.go within the payroll package, making i...
Learnt from: webpiratt
PR: vultisig/plugin#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.
Applied to files:
plugin/fees/constraints.goplugin/fees/helper.go
📚 Learning: in plugin/payroll/transaction.go, the gettokenid function is designed to return evm.zeroaddress.hex(...
Learnt from: webpiratt
PR: vultisig/plugin#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.
Applied to files:
plugin/fees/helper.goplugin/fees/transaction.go
📚 Learning: in the `tx_indexer` table (storage/postgres/migrations/plugin/*_tx_indexer.sql), the `policy_id` col...
Learnt from: webpiratt
PR: vultisig/plugin#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)`.
Applied to files:
storage/postgres/schema/schema.sqlstorage/postgres/migrations/plugin/20250630152230_fee_runs.sql
📚 Learning: in internal/keysign/signer.go, the `continue` statement in the checkkeysigncomplete error handling (...
Learnt from: webpiratt
PR: vultisig/plugin#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.
Applied to files:
plugin/fees/transaction.go
🧬 Code Graph Analysis (6)
storage/db.go (2)
internal/types/fees.go (3)
FeeRunState(11-11)FeeRun(29-39)Fee(21-26)internal/verifierapi/fees.go (1)
FeeDto(12-21)
plugin/fees/fees.go (1)
internal/types/fees.go (2)
FeeRunStateDraft(14-14)FeeRun(29-39)
internal/verifierapi/fees.go (1)
internal/verifierapi/verifierapi.go (1)
VerifierApi(26-31)
plugin/fees/post_tx.go (2)
plugin/fees/fees.go (1)
FeePlugin(36-51)internal/types/fees.go (3)
FeeRunStateSent(15-15)FeeRunStateDraft(14-14)FeeRun(29-39)
plugin/fees/transaction.go (3)
plugin/fees/fees.go (1)
FeePlugin(36-51)internal/types/fees.go (1)
FeeRun(29-39)internal/types/keysign.go (1)
PluginKeysignRequest(7-12)
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)
🪛 YAMLlint (1.37.1)
etc/vultisig/fee.yml
[error] 16-16: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (25)
internal/types/fees.go (3)
14-18: LGTM! Well-structured fee run state expansion.The addition of
FeeRunStateSuccess("completed") andFeeRunStateFailed("failed") states provides better granularity for tracking the fee run lifecycle beyond the existing "draft" and "sent" states.
34-34: Good architectural change from UUID to transaction hash.The migration from
TxID *uuid.UUIDtoTxHash *stringaligns well with blockchain transaction identification patterns, where transaction hashes are the natural identifiers for on-chain transactions.
38-38: Excellent addition of direct fee association.The new
Fees []Feefield enables direct association of multiple fees with a fee run, supporting the refactored workflow where fees are managed in batches per run.service/policy.go (1)
133-133: LGTM! Clean method name refactoring.The change from
GetAllPluginPoliciestoGetPluginPoliciesaligns with the database layer refactoring while maintaining the same functionality and signature.etc/vultisig/fee.yml (1)
5-6: LGTM! Good configuration structure for Ethereum provider.The
eth_providerURL andchain_id: 1configuration aligns with the architectural changes and supports Ethereum mainnet operations.storage/postgres/migrations/plugin/20250630152230_fee_runs.sql (2)
9-9: Perfect column specification for transaction hashes.The
tx_hash VARCHAR(66)column specification is correctly sized for Ethereum transaction hashes (0x prefix + 64 hex characters) and appropriately removes the foreign key constraint since transaction hashes are external blockchain identifiers.
28-34: LGTM! View properly updated for new schema.The
fee_run_with_totalsview is correctly updated to select and group bytx_hashinstead oftx_id, maintaining consistency with the table structure changes.internal/verifierapi/verifierapi.go (2)
4-5: LGTM! Appropriate imports for JSON POST functionality.The new imports
bytesandencoding/jsonare correctly added to support the JSON marshaling and HTTP POST request functionality.
55-70: Excellent implementation of authenticated POST method.The
postAuthmethod is well-implemented with:
- Proper JSON marshaling with error handling
- Correct HTTP request construction
- Appropriate headers (Content-Type and Authorization)
- Consistent pattern with existing
getAuthmethodThis provides the necessary functionality for fee collection API interactions.
storage/postgres/schema/schema.sql (2)
74-74: LGTM! Schema change supports the new transaction hash workflow.The change from
tx_idUUID with foreign key constraint totx_hashvarchar(66) is appropriate for storing Ethereum transaction hashes and aligns with the new fee processing workflow.
84-90: LGTM! View correctly updated to reflect schema changes.The
fee_run_with_totalsview has been properly updated to select and group bytx_hashinstead oftx_id, maintaining consistency with the table schema changes.internal/verifierapi/fees.go (1)
60-84: LGTM! Well-implemented API method.The
MarkFeeAsCollectedmethod is properly implemented with:
- Clear method signature using variadic parameters for fee IDs
- Structured request body with appropriate field names
- Proper error handling and HTTP status code validation
- Correct resource cleanup with deferred response body closing
storage/postgres/policy.go (1)
41-71: LGTM! Database method correctly implemented.The
GetAllFeePoliciesmethod is well-implemented with:
- Correct hardcoded plugin_id
'vultisig-fees-feee'(matches external dependency spelling)- Proper use of
DISTINCT ON(public_key)withORDER BYto get the latest policy per public key- Consistent error handling and row scanning pattern
- Good use of singular "Fee" in method name
cmd/fees/worker/main.go (3)
25-40: LGTM! Helper function properly implements fee loading.The
startLoadingFeesfunction correctly:
- Marshals the fee collection configuration into JSON payload
- Uses appropriate task enqueueing settings (zero retries, timeouts, retention)
- Includes proper error handling for marshaling failures
176-178: LGTM! Task handlers correctly registered.The mux handlers are properly registered with the new task type constants and corresponding fee plugin methods, supporting the 3-stage fee processing workflow.
180-199: Well-implemented cron scheduling system.The three separate cron schedulers are properly configured with:
- Configuration-driven cron expressions from
feePluginConfig.Jobs- Appropriate helper function calls for each stage
- Clear comments explaining the purpose of each scheduler
plugin/fees/transaction.go (1)
187-193: Good error handling patternExcellent use of
errors.Jointo aggregate multiple decoding errors while maintaining individual error context.plugin/fees/config.go (1)
52-54: Verify default cron expressions align with business requirementsThe default cron expressions show significant differences:
- Load: Every 2 minutes
- Transact: Weekly on Fridays at noon
- Post: Every 5 minutes
The transact job running only once per week seems unusual for a fee collection system.
Please confirm these default schedules align with business requirements, particularly the weekly transaction schedule.
storage/postgres/fees.go (5)
60-66: LGTM!The method correctly updates the fee run status and transaction hash. Good use of the status constant.
68-74: LGTM!Clean implementation for updating fee run status to success.
115-120: Verify map value modification patternThe code modifies a copy of the struct from the map and then reassigns it. While this works correctly in Go, it's a common source of confusion.
The current implementation is correct, but consider using pointers in the map for clarity:
rm := make(map[uuid.UUID]*types.FeeRun)
130-148: LGTM!Clean implementation for fetching fees by IDs. Good use of ANY clause for efficient batch queries.
168-174: LGTM!Simple and correct implementation for creating individual fees.
storage/db.go (2)
19-20: LGTM - Method rename improves clarityThe rename from
GetAllPluginPoliciestoGetPluginPoliciesbetter reflects that the method can filter results, not just return all policies.
25-32: Well-designed fee management interfaceThe new fee-related methods provide a comprehensive API for fee run lifecycle management:
- Creation with variadic fees parameter for flexibility
- Status updates (sent, success)
- Multiple query methods for different use cases
- Proper use of transaction hash instead of UUID
The interface follows good naming conventions using "fee" singular as per project guidelines.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
plugin/fees/transaction.go (1)
151-154: Intentional "not implemented" for run-based architectureAs documented in the learnings, the public
ProposeTransactionsmethod returning "not implemented" is intentional since the fees plugin v2 uses a run-based architecture with the privateproposeTransactionsmethod.storage/postgres/fees.go (1)
156-157: Consider using FeeRunStateDraft constantFor consistency with other status references in the codebase, consider using the constant instead of hardcoding the status value.
- query := `select id, status, created_at, updated_at, tx_hash, policy_id, total_amount, fee_count from fee_run_with_totals where status = $1 and policy_id = $2 order by created_at desc limit 1` - rows, err := p.pool.Query(ctx, query, types.FeeRunStateDraft, policyId) + query := `select id, status, created_at, updated_at, tx_hash, policy_id, total_amount, fee_count from fee_run_with_totals where status = $1 and policy_id = $2 order by created_at desc limit 1` + rows, err := p.pool.Query(ctx, query, types.FeeRunStateDraft, policyId)plugin/fees/fees.go (4)
69-69: Reuse the existing Ethereum clientThe
rpcClientis already created and can be reused instead of storing it separately asethClient.- rpcClient, err := ethclient.Dial(feeConfig.EthProvider) // ... other code ... - ethClient: rpcClient, + ethClient: rpcClient, // Already created aboveAlso applies to: 108-108
138-138: Use the provided context for semaphore acquisitionUsing
context.Background()ignores cancellation signals from the parent context, which could lead to goroutines continuing to run after the parent context is cancelled.- if err := sem.Acquire(ctx, 1); err != nil { + if err := sem.Acquire(ctx, 1); err != nil {
219-221: Add error handling for CreateFeeThe error from
CreateFeeshould be handled to avoid silent failures when adding fees to existing runs.- if err := fp.db.CreateFee(ctx, run.ID, fee); err != nil { - return fmt.Errorf("failed to create fee: %w", err) - } + if err := fp.db.CreateFee(ctx, run.ID, fee); err != nil { + return fmt.Errorf("failed to add fee to fee run: %w", err) + }
284-284: Use the provided context for semaphore acquisitionSimilar to the previous issue, use the provided context instead of
context.Background().- if err := sem.Acquire(ctx, 1); err != nil { + if err := sem.Acquire(ctx, 1); err != nil {
🧹 Nitpick comments (5)
etc/vultisig/fee.yml (1)
16-16: Add missing newline at end of fileYAML files should end with a newline character.
post: cronexpr: "@every 5m" max_concurrent_jobs: 10 +internal/types/fees.go (1)
34-38: Consider documenting the relationship between TotalAmount and FeesThe
FeeRunstruct has both aTotalAmountfield and aFeesslice. It would be helpful to document whetherTotalAmountis derived from summing theFeesamounts or if it can differ.Add a comment to clarify the relationship:
type FeeRun struct { ID uuid.UUID `db:"id"` Status FeeRunState `db:"status"` CreatedAt time.Time `db:"created_at"` UpdatedAt time.Time `db:"updated_at"` TxHash *string `db:"tx_hash"` PolicyID uuid.UUID `db:"policy_id"` + // TotalAmount is the sum of all Fee.Amount values in Fees slice TotalAmount int `db:"total_amount"` FeeCount int `db:"fee_count"` Fees []Fee `db:"fees"` }plugin/fees/config.go (3)
19-21: Consider making chainId field consistent with naming convention.The struct has both
chainId(unexported, mapstructure tag) andChainId(exported) fields for the same data. While this works functionally, it creates potential confusion about which field to use.Consider renaming the internal field to follow Go conventions (e.g.,
internalChainIdorrawChainId) to make the distinction clearer.- chainId uint64 `mapstructure:"chain_id"` // The chain ID of the Ethereum blockchain. + rawChainId uint64 `mapstructure:"chain_id"` // The chain ID of the Ethereum blockchain.And update line 129 accordingly:
- c.ChainId = big.NewInt(0).SetUint64(c.chainId) + c.ChainId = big.NewInt(0).SetUint64(c.rawChainId)
24-24: Fix comment typo: "consecutive" should be "concurrent".The comment mentions "consecutive tasks" but based on the field name
MaxConcurrentJobs, it should be "concurrent tasks".- MaxConcurrentJobs uint64 `mapstructure:"max_concurrent_jobs"` //How many consecutive tasks can take place + MaxConcurrentJobs uint64 `mapstructure:"max_concurrent_jobs"` //How many concurrent tasks can take placeApply the same fix to lines 28 and 34.
Also applies to: 28-28, 34-34
32-32: Misleading comment for SuccessConfirmations field.The comment "How many consecutive tasks can take place" doesn't match the field name
SuccessConfirmations, which typically refers to the number of block confirmations needed before considering a transaction successful.- SuccessConfirmations uint64 `mapstructure:"success_confirmations"` //How many consecutive tasks can take place + SuccessConfirmations uint64 `mapstructure:"success_confirmations"` //How many block confirmations required for transaction success
📜 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 (9)
- service/policy.go
- plugin/fees/helper.go
- storage/postgres/schema/schema.sql
- plugin/fees/post_tx.go
- internal/verifierapi/verifierapi.go
- storage/postgres/policy.go
- internal/verifierapi/fees.go
- plugin/fees/constraints.go
- cmd/fees/worker/main.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-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/config.gostorage/postgres/fees.goplugin/fees/transaction.goplugin/fees/fees.goetc/vultisig/fee.yml
📚 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:
storage/postgres/fees.goplugin/fees/transaction.goplugin/fees/fees.go
📚 Learning: 2025-06-11T18:42:54.241Z
Learnt from: webpiratt
PR: vultisig/plugin#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)`.
Applied to files:
storage/postgres/migrations/plugin/20250630152230_fee_runs.sql
📚 Learning: 2025-07-24T19:34:47.441Z
Learnt from: webpiratt
PR: vultisig/plugin#141
File: plugin/payroll/transaction.go:208-267
Timestamp: 2025-07-24T19:34:47.441Z
Learning: In the vultisig/plugin codebase, duplicate transaction prevention for the payroll plugin has been moved to the verifier side, so the local `IsAlreadyProposed` check in plugin/payroll/transaction.go is no longer needed. The validation flow has changed and is now handled by the verifier rather than within the plugin itself.
Applied to files:
plugin/fees/transaction.go
📚 Learning: 2025-07-19T12:28:15.328Z
Learnt from: webpiratt
PR: vultisig/plugin#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.
Applied to files:
plugin/fees/transaction.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-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/transaction.go
🧬 Code Graph Analysis (4)
storage/db.go (2)
internal/types/fees.go (3)
FeeRunState(11-11)FeeRun(29-39)Fee(21-26)internal/verifierapi/fees.go (1)
FeeDto(12-21)
storage/postgres/fees.go (3)
storage/postgres/db.go (1)
PostgresBackend(15-17)internal/types/fees.go (6)
FeeRunState(11-11)FeeRun(29-39)FeeRunStateSent(15-15)FeeRunStateSuccess(16-16)Fee(21-26)FeeRunStateDraft(14-14)internal/verifierapi/fees.go (1)
FeeDto(12-21)
plugin/fees/transaction.go (3)
plugin/fees/fees.go (1)
FeePlugin(36-51)internal/types/fees.go (1)
FeeRun(29-39)internal/types/keysign.go (1)
PluginKeysignRequest(7-12)
plugin/fees/fees.go (1)
internal/types/fees.go (2)
FeeRunStateDraft(14-14)FeeRun(29-39)
🪛 YAMLlint (1.37.1)
etc/vultisig/fee.yml
[error] 16-16: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (16)
storage/postgres/migrations/plugin/20250630152230_fee_runs.sql (2)
9-9: LGTM! Appropriate column type for Ethereum transaction hashesUsing
VARCHAR(66)fortx_hashis correct for storing Ethereum transaction hashes with the '0x' prefix (2 chars) + 64 hex characters.
37-37: Index on status column will improve query performanceGood addition of the index on
fee_run(status)as it will speed up queries filtering by status, which appears to be a common operation based on the new methods likeGetFeeRunsandGetAllFeeRuns.internal/types/fees.go (1)
14-17: Well-structured state machine for fee runsThe fee run states follow a clear progression: draft → sent → completed/failed. This provides good visibility into the transaction lifecycle.
storage/db.go (2)
19-20: Clean API separation for fee policiesThe separation of
GetPluginPolicies(filtered by plugin type) andGetAllFeePolicies(specifically for fees) provides a clear API surface.
25-32: Comprehensive fee run management APIThe new methods provide a complete API for the fee run lifecycle - from creation through transaction sending to completion. The variadic parameters in
CreateFeeRunand filtering options inGetAllFeeRunsare well-designed.plugin/fees/transaction.go (1)
189-192: Robust transaction hash calculationThe transaction hash calculation using
getHashproperly handles the transaction bytes and signature components to derive the final hash before broadcasting.storage/postgres/fees.go (1)
108-109: Efficient fee filtering by run IDsGood optimization to filter fees by the relevant run IDs instead of fetching all fees from the database.
plugin/fees/fees.go (1)
288-293: Error from executeFeeTransaction is properly handledThe error from
executeFeeTransactionis correctly logged and returned to the errgroup, ensuring failures are propagated.plugin/fees/config.go (8)
22-36: Well-structured job configuration design.The nested Jobs structure with Load, Transact, and Post stages provides clear separation of concerns and allows for fine-grained control over each stage's concurrency and scheduling. The mapstructure tags are properly configured for configuration unmarshalling.
47-50: Good default values for job concurrency and confirmations.The default values are reasonable:
- 10 concurrent jobs for each stage provides good parallelism without overwhelming the system
- 20 success confirmations is a conservative choice for Ethereum transaction finality
85-92: LGTM: Clean helper function for job concurrency configuration.The
WithJobConcurrencyfunction provides a convenient way to set all three job concurrency limits in one call, which is useful for testing and configuration.
94-101: LGTM: Consistent helper function for cron expressions.The
WithCronexprfunction follows the same pattern asWithJobConcurrency, providing a clean way to configure all three job schedules simultaneously.
129-129: *LGTM: Proper conversion from uint64 to big.Int.The conversion from the internal
chainIdfield to the exportedChainIdfield is handled correctly, ensuring the big.Int is properly initialized.
156-158: LGTM: Essential validation for Ethereum provider.The validation ensures
EthProvideris configured, which is critical for the fees plugin to interact with the Ethereum blockchain.
160-167: LGTM: Reasonable bounds for concurrent job limits.The validation ensures all job types have concurrency between 1 and 100, which prevents both misconfiguration (0 jobs) and potential resource exhaustion (too many concurrent jobs).
52-54: Cron expression confirmed as intentionalThe
0 12 * * 5schedule inplugin/fees/config.gois explicitly documented in the codebase, indicating it was an intentional choice:
- cmd/fees/worker/main.go:
// Transact fees every Friday at 12:00 PMNo changes are required.
| if err := fp.db.SetFeeRunSent(ctx, runId, tx.Hash().Hex()); err != nil { //TODO pass the real tx id | ||
| return fmt.Errorf("failed to set fee run sent: %w", err) | ||
| } |
There was a problem hiding this comment.
Critical: Ensure fee run status is updated after broadcast
The comment correctly highlights the importance of this operation. If SetFeeRunSent fails after the transaction is broadcast, the fee run will remain in draft status despite the transaction being on-chain, causing potential double-spending issues.
Consider wrapping this in a retry mechanism or implementing a reconciliation process:
- if err := fp.db.SetFeeRunSent(ctx, runId, tx.Hash().Hex()); err != nil { //TODO pass the real tx id
- return fmt.Errorf("failed to set fee run sent: %w", err)
+ // Retry up to 3 times to ensure the status is updated
+ var updateErr error
+ for i := 0; i < 3; i++ {
+ if updateErr = fp.db.SetFeeRunSent(ctx, runId, tx.Hash().Hex()); updateErr == nil {
+ break
+ }
+ fp.logger.WithError(updateErr).WithField("attempt", i+1).Warn("Failed to update fee run status, retrying...")
+ }
+ if updateErr != nil {
+ // Log critical error - transaction was sent but status not updated
+ fp.logger.WithFields(logrus.Fields{
+ "runId": runId,
+ "txHash": tx.Hash().Hex(),
+ "error": updateErr,
+ }).Error("CRITICAL: Transaction broadcast but fee run status not updated")
+ return fmt.Errorf("failed to set fee run sent after transaction broadcast: %w", updateErr)
}📝 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.
| if err := fp.db.SetFeeRunSent(ctx, runId, tx.Hash().Hex()); err != nil { //TODO pass the real tx id | |
| return fmt.Errorf("failed to set fee run sent: %w", err) | |
| } | |
| // Retry up to 3 times to ensure the status is updated | |
| var updateErr error | |
| for i := 0; i < 3; i++ { | |
| if updateErr = fp.db.SetFeeRunSent(ctx, runId, tx.Hash().Hex()); updateErr == nil { | |
| break | |
| } | |
| fp.logger.WithError(updateErr).WithField("attempt", i+1).Warn("Failed to update fee run status, retrying...") | |
| } | |
| if updateErr != nil { | |
| // Log critical error - transaction was sent but status not updated | |
| fp.logger.WithFields(logrus.Fields{ | |
| "runId": runId, | |
| "txHash": tx.Hash().Hex(), | |
| "error": updateErr, | |
| }).Error("CRITICAL: Transaction broadcast but fee run status not updated") | |
| return fmt.Errorf("failed to set fee run sent after transaction broadcast: %w", updateErr) | |
| } |
🤖 Prompt for AI Agents
In plugin/fees/transaction.go around lines 215 to 217, the call to
fp.db.SetFeeRunSent after broadcasting the tx can fail and leave the fee run in
draft; wrap this operation in a retry-with-backoff loop (e.g., 3-5 attempts with
exponential backoff), log each failure with context (runId and tx.Hash().Hex()),
and on final failure enqueue a durable reconciliation record (or persistent
retry queue) containing runId and tx hash so a background reconciler can mark
the run sent or alert operators; also replace the TODO by ensuring the real
transaction id is used when available and emit a metric/event on success and on
terminal failure for monitoring.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
etc/vultisig/fee.yml (1)
8-9: Good catch: removed hardcoded provider API keyReplacing the hardcoded Alchemy URL with an environment variable resolves the earlier secret exposure concern.
🧹 Nitpick comments (1)
etc/vultisig/fee.yml (1)
4-4: Optional: externalize verifier_token as a secretIf this token is used outside of local dev, consider moving it to an env var (e.g.,
${VERIFIER_TOKEN}) to avoid accidental exposure in different environments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
etc/vultisig/fee.yml(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 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-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:
etc/vultisig/fee.yml
🪛 YAMLlint (1.37.1)
etc/vultisig/fee.yml
[error] 7-7: syntax error: mapping values are not allowed here
(syntax)
🔇 Additional comments (1)
etc/vultisig/fee.yml (1)
10-20: Verify environment variable expansion and cron semantics
- In plugin/fees/config.go, the
mapstructuretags foreth_provider,chain_id,jobs,cronexpr, andmax_concurrent_jobscorrectly match the keys in your YAML.- Viper is set up with
AutomaticEnvand theFEES_prefix, soFEES_ETH_PROVIDERwill override the file. It will not perform inline expansion of${ETH_PROVIDER_URL}in the YAML value—if you need that, applyos.ExpandEnv(or equivalent) toEthProviderafter unmarshalling.- I didn’t locate the scheduler code in
plugin/fees; please verify which scheduler (e.g., asynq’s Scheduler or robfig/cron) is used forcronexprand ensure it supports both@every …and standard 5-field cron syntax, as well as the desired timezone semantics.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
etc/vultisig/fee.yml (1)
5-20:⚠️ Merge conflict markers committed into configuration fileThe configuration file contains git merge conflict markers (lines 5-9 starting with
---,+++,@@) which break the YAML structure and will cause parsing failures at runtime.Apply this diff to fix the file:
---- etc/vultisig/fee.yml -+++ etc/vultisig/fee.yml -@@ Lines: 5-5 --eth_provider: https://eth-mainnet.g.alchemy.com/v2/HAtIwB5y82TNVwaHkwcMj -+eth_provider: ${ETH_PROVIDER_URL} +eth_provider: ${ETH_PROVIDER_URL} chain_id: 1 jobs: - load: - cronexpr: "@every 2m" - max_concurrent_jobs: 10 - transact: - cronexpr: "0 12 * * 5" - max_concurrent_jobs: 10 - post: - cronexpr: "@every 5m" - max_concurrent_jobs: 10 + load: + cronexpr: "@every 2m" + max_concurrent_jobs: 10 + transact: + cronexpr: "0 12 * * 5" + max_concurrent_jobs: 10 + post: + cronexpr: "@every 5m" + max_concurrent_jobs: 10cmd/fees/worker/main.go (1)
25-40: Inconsistent payload handling between fee tasksThe
LoadFeeshandler inplugin/fees/fees.gocompletely ignores the task payload, yetstartLoadingFeesmarshals a structuredFeeCollectionFormatpayload. This creates unnecessary complexity and confusion.Since the payload is ignored, simplify by using an empty payload like the other tasks:
func startLoadingFees(asynqClient *asynq.Client, logger *logrus.Logger) { logger.Info("Loading fees") - payload, err := json.Marshal(fees.FeeCollectionFormat{ - FeeCollectionType: fees.FeeCollectionTypeAll, - }) - if err != nil { - logger.WithError(err).Error("Failed to marshal fee loading config in demo run") - return - } + payload := []byte{} asynqClient.Enqueue( asynq.NewTask(fees.TypeFeeLoad, payload),plugin/fees/fees.go (4)
69-72: Remove duplicate Ethereum client initializationThe Ethereum client is created as
rpcClientat line 69 and stored inethClientat line 108. This is the same client being used twice.Since both fields use the same client, consider removing the duplicate field or clearly documenting why both are needed if there's a specific reason.
Also applies to: 108-108
138-140: Use the provided context for semaphore acquisitionUsing
context.Background()instead of the providedctxmeans cancellation signals won't be properly propagated, potentially causing goroutines to continue running after the parent context is cancelled.- if err := sem.Acquire(context.Background(), 1); err != nil { + if err := sem.Acquire(ctx, 1); err != nil {
219-221: Add error handling for CreateFeeThe error returned by
CreateFeeshould be handled to avoid silent failures when adding fees to existing runs.The error is already being handled correctly in the current code. The method properly returns the error if
CreateFeefails.
284-286: Use the provided context for semaphore acquisitionSimilar to the issue in
LoadFees, usingcontext.Background()prevents proper cancellation propagation.- if err := sem.Acquire(context.Background(), 1); err != nil { + if err := sem.Acquire(ctx, 1); err != nil {
🧹 Nitpick comments (8)
cmd/fees/worker/main.go (3)
180-186: Inconsistent comment about scheduling frequencyThe comment states "Load fees every 10 minutes" but the actual cron expression is configured in the YAML file as "@every 2m" (every 2 minutes).
Update the comment to be more generic:
- // Load fees every 10 minutes + // Load fees based on configured schedule
187-193: Update comment to be configuration-awareThe comment states "Transact fees every Friday at 12:00 PM" but the actual schedule is determined by the configuration.
- // Transact fees every Friday at 12:00 PM + // Transact fees based on configured schedule
194-199: Fix misleading commentThe comment states "Update verifier" but this actually handles post-transaction processing.
- // // Update verifier every 10 minutes + // Post-transaction processing based on configured schedulestorage/postgres/fees.go (2)
15-58: Consider using a database transaction for the entire CreateFeeRun operationThe method begins a transaction at line 28 but performs the policy validation query outside of it (lines 17-26). If another process deletes or modifies the policy between the validation and the transaction, it could lead to inconsistent state.
Move the policy validation inside the transaction:
func (p *PostgresBackend) CreateFeeRun(ctx context.Context, policyId uuid.UUID, state types.FeeRunState, fees ...verifierapi.FeeDto) (*types.FeeRun, error) { - // Check policy id is valid - query := `select plugin_id from plugin_policies where id = $1` - policyrows := p.pool.QueryRow(ctx, query, policyId) - var pluginId string - err := policyrows.Scan(&pluginId) - if err != nil { - return nil, err - } - if pluginId != "vultisig-fees-feee" { - return nil, errors.New("plugin id not found or not vultisig-fees-feee") - } - tx, err := p.pool.Begin(ctx) if err != nil { return nil, err } defer tx.Rollback(ctx) + + // Check policy id is valid within the transaction + query := `select plugin_id from plugin_policies where id = $1` + var pluginId string + err = tx.QueryRow(ctx, query, policyId).Scan(&pluginId) + if err != nil { + return nil, err + } + if pluginId != "vultisig-fees-feee" { + return nil, errors.New("plugin id not found or not vultisig-fees-feee") + } + runId := uuid.New()
76-133: Consider optimizing fee fetching for large datasetsWhen fetching fee runs with many associated fees, the current implementation makes N+1 queries (one for runs, one for all fees). While the fee query is now properly filtered by run IDs, consider using a JOIN query for better performance with large datasets.
Consider using a single query with JSON aggregation:
SELECT fr.id, fr.status, fr.created_at, fr.updated_at, fr.tx_hash, fr.policy_id, fr.total_amount, fr.fee_count, COALESCE(json_agg( json_build_object( 'id', f.id, 'fee_run_id', f.fee_run_id, 'amount', f.amount ) ORDER BY f.created_at ) FILTER (WHERE f.id IS NOT NULL), '[]') as fees FROM fee_run_with_totals fr LEFT JOIN fee f ON f.fee_run_id = fr.id WHERE fr.status = ANY($1) GROUP BY fr.id, fr.status, fr.created_at, fr.updated_at, fr.tx_hash, fr.policy_id, fr.total_amount, fr.fee_countplugin/fees/config.go (3)
22-36: Fix comment inconsistencies in Jobs struct field documentation.Several comments contain inaccuracies:
- Line 24, 28: "consecutive tasks" should be "concurrent tasks"
- Line 25, 29: "Cron link expression" should be "Cron expression"
- Line 32: Comment describes "How many consecutive tasks" but field name is
SuccessConfirmationswhich represents confirmation count, not task countApply this diff to fix the comment inconsistencies:
- MaxConcurrentJobs uint64 `mapstructure:"max_concurrent_jobs"` //How many consecutive tasks can take place - Cronexpr string `mapstructure:"cronexpr"` // Cron link expression on how often these tasks should run + MaxConcurrentJobs uint64 `mapstructure:"max_concurrent_jobs"` // How many concurrent tasks can take place + Cronexpr string `mapstructure:"cronexpr"` // Cron expression on how often these tasks should run- MaxConcurrentJobs uint64 `mapstructure:"max_concurrent_jobs"` //How many consecutive tasks can take place - Cronexpr string `mapstructure:"cronexpr"` // Cron link expression on how often these tasks should run + MaxConcurrentJobs uint64 `mapstructure:"max_concurrent_jobs"` // How many concurrent tasks can take place + Cronexpr string `mapstructure:"cronexpr"` // Cron expression on how often these tasks should run- SuccessConfirmations uint64 `mapstructure:"success_confirmations"` //How many consecutive tasks can take place - Cronexpr string `mapstructure:"cronexpr"` // Cron link expression on how often these tasks should run + SuccessConfirmations uint64 `mapstructure:"success_confirmations"` // Number of confirmations required for transaction success + Cronexpr string `mapstructure:"cronexpr"` // Cron expression on how often these tasks should run
19-21: Consider making chainId field private and adding validation.The dual field approach (unexported
chainIdand exportedChainId) is reasonable for unmarshalling, but consider adding validation to ensure the conversion is safe and prevent integer overflow scenarios.Consider adding validation in the
WithFileConfigfunction:+ // Validate chainId before conversion to prevent overflow + if c.chainId == 0 { + return errors.New("chain_id must be greater than 0") + } + c.ChainId = big.NewInt(0).SetUint64(c.chainId)
71-76: Consider renaming function to match the field it configures.The function
WithEthClientsets theEthProviderfield, which creates a semantic mismatch. Consider renaming for clarity.Apply this diff to rename the function:
-func WithEthClient(url string) ConfigOption { +func WithEthProvider(url string) ConfigOption {
📜 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 (10)
- internal/types/fees.go
- storage/postgres/schema/schema.sql
- storage/postgres/policy.go
- storage/postgres/migrations/plugin/20250630152230_fee_runs.sql
- plugin/fees/helper.go
- service/policy.go
- plugin/fees/post_tx.go
- internal/verifierapi/verifierapi.go
- plugin/fees/constraints.go
- plugin/fees/transaction.go
🧰 Additional context used
🧠 Learnings (4)
📓 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-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:
cmd/fees/worker/main.gostorage/postgres/fees.goplugin/fees/config.goplugin/fees/fees.goetc/vultisig/fee.yml
📚 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:
cmd/fees/worker/main.goplugin/fees/fees.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:
cmd/fees/worker/main.gostorage/postgres/fees.goplugin/fees/fees.go
🧬 Code Graph Analysis (4)
internal/verifierapi/fees.go (1)
internal/verifierapi/verifierapi.go (1)
VerifierApi(26-31)
cmd/fees/worker/main.go (2)
plugin/fees/config.go (3)
FeeCollectionFormat(188-191)FeeCollectionType(179-179)FeeCollectionTypeAll(185-185)plugin/fees/constraints.go (3)
TypeFeeLoad(6-6)TypeFeeTransact(7-7)TypeFeePostTx(8-8)
storage/postgres/fees.go (3)
storage/postgres/db.go (1)
PostgresBackend(15-17)internal/types/fees.go (6)
FeeRunState(11-11)FeeRun(29-39)FeeRunStateSent(15-15)FeeRunStateSuccess(16-16)Fee(21-26)FeeRunStateDraft(14-14)internal/verifierapi/fees.go (1)
FeeDto(12-21)
storage/db.go (2)
internal/types/fees.go (3)
FeeRunState(11-11)FeeRun(29-39)Fee(21-26)internal/verifierapi/fees.go (1)
FeeDto(12-21)
🪛 YAMLlint (1.37.1)
etc/vultisig/fee.yml
[error] 7-7: syntax error: mapping values are not allowed here
(syntax)
⏰ 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 (9)
internal/verifierapi/fees.go (1)
60-84: LGTM! Well-structured fee collection marking endpoint.The implementation follows good practices with proper error handling, response body cleanup via defer, and clear status code validation.
plugin/fees/fees.go (1)
288-293: Consider how to handle transaction execution errorsWhen
executeFeeTransactionfails, the error is logged and returned to the error group, which will causeHandleTransactionsto return an error. Consider whether failed transactions should mark the fee run with a failed status to prevent repeated processing attempts.How are failed fee runs tracked and retried? Should there be a mechanism to mark runs as failed to prevent infinite retry loops?
plugin/fees/config.go (2)
160-167: LGTM! Comprehensive validation for job concurrency limits.The validation correctly ensures all job types have reasonable concurrency limits between 1 and 100, preventing resource exhaustion while ensuring at least one concurrent job can execute.
47-54: Confirm Cron Expression Parsing & Production FitI’ve verified with robfig/cron/v3’s parser that all three expressions are syntactically valid. Please ensure these defaults align with your production workload:
File: plugin/fees/config.go (lines 47–54)
c.Jobs.Load.Cronexpr = "@every 2m"c.Jobs.Post.Cronexpr = "@every 5m"c.Jobs.Transact.Cronexpr = "0 12 * * 5"(Fridays at noon)storage/db.go (5)
19-20: LGTM! Method rename improves API consistency.The rename from
GetAllPluginPoliciestoGetPluginPoliciesbetter reflects the method's filtering capabilities (by publicKey, pluginID, and active status) and aligns with the newGetAllFeePoliciesmethod that truly returns all fee policies.
25-25: Excellent variadic parameter improvement for CreateFeeRun.The change from
fees []verifierapi.FeeDtotofees ...verifierapi.FeeDtoprovides better API ergonomics while maintaining backward compatibility. Callers can now pass fees directly without constructing a slice.
26-26: Consistent transaction identifier type change.The change from
txId uuid.UUIDtotxHash stringaligns with the broader refactoring mentioned in the AI summary, replacing UUID-based transaction IDs with string-based transaction hashes throughout the system.
27-32: Well-designed fee run lifecycle management methods.The new methods provide comprehensive fee run management:
SetFeeRunSuccesscompletes the transaction lifecycleGetAllFeeRunswith optional status filtering supports monitoringGetFeesenables fee detail retrievalGetPendingFeeRunprevents duplicate runs per policyCreateFeesupports incremental fee additionGetFeeRunsprovides status-specific queriesThe API design supports the three-stage workflow (load, transact, post-tx) mentioned in the PR objectives.
28-28: GetAllFeeRuns variadic handling is correct – no action requiredThe implementation in
storage/postgres/fees.goclearly checkslen(statuses) == 0to return all fee runs when no states are passed, and appliesWHERE status = ANY($1)otherwise. The pluralstatusesparameter and accompanying comment are consistent.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (8)
plugin/fees/FEES.md (6)
7-7: Clarify system description: two components, three stagesCurrently says “2 part system” but the doc describes three stages (load, transact, post). Be explicit about both.
-As this is a 2 part system, and the fees plugin works differently from other plugins. Here is an overview of the current architecture. +This is a two-part system (Verifier + Plugin) with a three-stage workflow (Load, Transact, Post). The fee plugin works differently from other plugins. Here is an overview of the current architecture.
21-21: Tighten phrasing per style hintRemove wordiness and clarify the audit fields.
-Fees are known to be collected successfully by the use of a `collected_at` attribute and the `charged_at` as being set as the date the fee was incurred which is pre-empted to be needed for audit purposes. +Fees are marked as collected via `collected_at`. The `charged_at` field records when the fee was incurred for audit purposes.
42-43: Fix grammar and clarify separation of concernsImprove readability.
-The db structure are different between verifier and plugin here due to a separation of concerns. Verifier needs to create fee entries, track their state across their lifecycle and handle treasury output from them. The plugin server simply needs to group them and track their ids and amounts. +The DB structures differ between the verifier and the plugin due to separation of concerns. The verifier creates fee entries, tracks their lifecycle, and handles treasury accounting. The plugin groups fees into runs and tracks their IDs and amounts.
57-57: Fix mutual-exclusion phrasingThere are 3 stages total; Load should run only when the other two are not running.
-Will only run when one of the other 3 processes isn't running +Runs only when neither of the other two stages (transact/post) is running
80-83: Avoid hardcoding chainId and gas; make them config/estimatedReflects real behavior and avoids misleading implementers.
-- **Chain**: Ethereum (chainId: 1) +- **Chain**: Ethereum (chainId: configured; e.g., 1 for mainnet) @@ -- **Gas Limit**: 65,000 (typical ERC20 transfer upper bound) +- **Gas**: Estimated at runtime with a safety cap (typical ERC20 transfer ≈ 50–65k)
135-141: Use consistent logging field casingAll fields use lowerCamel except tx_hash. Align for consistency.
-- `tx_hash`: Blockchain transaction hash +- `txHash`: Blockchain transaction hashetc/vultisig/fee.yml (2)
14-16: Optionally expose success confirmations for post stageIf the code supports a Post.SuccessConfirmations setting, surface it here instead of relying on an implicit default.
Apply:
post: cronexpr: "@every 5m" max_concurrent_jobs: 10 + success_confirmations: 3If 3 isn’t the intended default, adjust accordingly.
16-16: Add a trailing newlineYAMLlint flagged the missing EOF newline. Add one to satisfy linters and POSIX text-file conventions.
Apply:
- max_concurrent_jobs: 10 + max_concurrent_jobs: 10 +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
etc/vultisig/fee.yml(1 hunks)plugin/fees/FEES.md(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 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-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/FEES.md
📚 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.mdetc/vultisig/fee.yml
📚 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.md
🪛 LanguageTool
plugin/fees/FEES.md
[style] ~21-~21: ‘by the use of’ might be wordy. Consider a shorter alternative.
Context: ... are known to be collected successfully by the use of a collected_at attribute and the `cha...
(EN_WORDINESS_PREMIUM_BY_THE_USE_OF)
🪛 YAMLlint (1.37.1)
etc/vultisig/fee.yml
[error] 16-16: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (2)
etc/vultisig/fee.yml (2)
5-6: Good move: replace hardcoded provider URL with an environment variableSwitching from an embedded API key to an env var removes secrets from VCS and aligns with best practices.
3-6: Sanity-check chain_id vs USDC addressThe USDC address provided is mainnet. Confirm chain_id: 1 is intentional and test/staging environments won’t pair a testnet chain_id with a mainnet address.
If you need environment-specific configs, consider separate YAMLs or templating to avoid cross‑environment misconfigurations.
| - 0x5BB06B9C5e4f7624Df7100Badb2F5AA1C86d8498 | ||
| collector_address: 0x7d760c17d798a7A9a4c4AcAf311A02dC95972503 | ||
| usdc_address: 0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48 | ||
| verifier_token: localhost-fee-apikey |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t commit verifier tokens; parameterize via environment
Even placeholder API keys can leak into logs/config dumps. Prefer env or secret manager injection.
Apply:
-verifier_token: localhost-fee-apikey
+verifier_token: ${FEES_VERIFIER_TOKEN}If you don’t perform env expansion in YAML, read this from env in code instead.
📝 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.
| verifier_token: localhost-fee-apikey | |
| verifier_token: ${FEES_VERIFIER_TOKEN} |
🤖 Prompt for AI Agents
In etc/vultisig/fee.yml around line 4, the verifier_token value is a committed
API key placeholder; remove this hard-coded token and parameterize it so the
runtime reads it from an environment variable or secret manager instead. Replace
the literal verifier_token entry with a reference (or remove the field) and
update application config loading to read process environment (or secret store)
e.g., load VERIFIER_TOKEN at startup, validate presence and fail fast if
missing, and ensure any deployment manifests/CI inject the secret rather than
committing it to repo.
| collector_address: 0x7d760c17d798a7A9a4c4AcAf311A02dC95972503 | ||
| usdc_address: 0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48 | ||
| verifier_token: localhost-fee-apikey | ||
| eth_provider: ${ETH_PROVIDER_URL} |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure env var expansion is actually performed for eth_provider
Many YAML loaders don’t expand ${...} by default. If the config loader doesn’t explicitly expand env vars, this will be treated as the literal string "${ETH_PROVIDER_URL}" at runtime.
Run this to verify the codebase performs env expansion or reads eth_provider from env:
If env expansion isn’t present, either:
- Expand at load-time in code (preferred), or
- Document/ensure deployment runs envsubst on the file before loading.
🏁 Script executed:
#!/bin/bash
# Look for explicit env expansion or reading of ETH_PROVIDER_URL / eth_provider
rg -n 'os\.ExpandEnv|os\.Getenv|ExpandString|envsubst|ETH_PROVIDER_URL|eth_provider' -A2
# Inspect fees config parsing for struct tags and env handling
rg -n 'EthProvider|ChainId|cronexpr|max_concurrent_jobs|success_confirmations' -A4Length of output: 8970
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- viper occurrences ----"
rg -n "viper\." -S || true
echo
echo "---- config-related functions (ReadInConfig, Unmarshal, AutomaticEnv, SetEnvPrefix, SetEnvKeyReplacer) ----"
rg -n "ReadInConfig|Unmarshal|AutomaticEnv|SetEnvPrefix|SetEnvKeyReplacer|mapstructure" -S || true
echo
echo "---- plugin/fees/config.go ----"
sed -n '1,240p' plugin/fees/config.go || true
echo
echo "---- cmd/fees/config/config.go ----"
sed -n '1,240p' cmd/fees/config/config.go || trueLength of output: 24792
Action required: make ETH provider expansion explicit (fix required)
Viper does not expand ${...} inside YAML. WithFileConfig in plugin/fees/config.go uses viper.AutomaticEnv() with prefix FEES, so the runtime env var that will override the key is FEES_ETH_PROVIDER — but the literal string "${ETH_PROVIDER_URL}" in etc/vultisig/fee.yml will not be expanded unless you preprocess the file or change the loader.
Files to address
- etc/vultisig/fee.yml — line with:
eth_provider: ${ETH_PROVIDER_URL} - plugin/fees/config.go — WithFileConfig (viper setup / ReadInConfig / Unmarshal)
Recommended fixes (pick one)
-
Quick/deployment fix: set FEES_ETH_PROVIDER in your environment (this will override the config value because of AutomaticEnv + SetEnvPrefix). Optionally change fee.yml to an explicit empty/placeholder value to avoid the ${...} illusion:
eth_provider: "" # overridden by FEES_ETH_PROVIDER -
Preferred code fix: expand env vars in the loaded config before unmarshalling. Replace the ReadInConfig+Unmarshal block with a read + os.ExpandEnv + v.ReadConfig approach, e.g.:
// after v.SetEnvKeyReplacer(...)
if err := v.ReadInConfig(); err != nil {
return fmt.Errorf("failed to read config: %w", err)
}
cfgPath := v.ConfigFileUsed()
if cfgPath != "" {
b, err := os.ReadFile(cfgPath)
if err != nil {
return fmt.Errorf("failed to read config for env expansion: %w", err)
}
expanded := os.ExpandEnv(string(b))
v.SetConfigType("yaml")
if err := v.ReadConfig(bytes.NewBufferString(expanded)); err != nil {
return fmt.Errorf("failed to read expanded config: %w", err)
}
}
if err := v.Unmarshal(c); err != nil {
return fmt.Errorf("failed to unmarshal config: %w", err)
}(Add imports: "os", "bytes")
Either document that deployments must run envsubst on fee.yml before starting, or implement the in-code expansion above. Leaving the current ${ETH_PROVIDER_URL} in the file without one of these changes will result in the literal string being used and NewFeeConfig failing the "eth_provider is required" check unless FEES_ETH_PROVIDER is set.
🤖 Prompt for AI Agents
In etc/vultisig/fee.yml around line 5 and plugin/fees/config.go in
WithFileConfig (viper setup / ReadInConfig / Unmarshal): the YAML contains a
literal "${ETH_PROVIDER_URL}" which Viper will not expand, so either set
FEES_ETH_PROVIDER in the environment (quick fix) or implement in-code env
expansion before Unmarshal (preferred) by reading the config file path via
v.ConfigFileUsed(), os.ReadFile it to a string, run os.ExpandEnv on the
contents, call v.SetConfigType("yaml") and
v.ReadConfig(bytes.NewBufferString(expanded)) and then v.Unmarshal(c); add
imports for "os" and "bytes".
| jobs: | ||
| load: | ||
| cronexpr: "@every 2m" | ||
| max_concurrent_jobs: 10 | ||
| transact: | ||
| cronexpr: "0 12 * * 5" | ||
| max_concurrent_jobs: 10 | ||
| post: | ||
| cronexpr: "@every 5m" | ||
| max_concurrent_jobs: 10 No newline at end of file |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm scheduler expects these keys and cron syntax
Validate that the config parser and scheduler expect:
- Keys: jobs.load|transact|post with fields cronexpr and max_concurrent_jobs
- Cron formats: robfig/cron/Asynq compatibility with "@every 2m" and "0 12 * * 5"
Also confirm concurrency here configures enqueuer concurrency (not worker pool), to avoid over-enqueueing or duplicate runs.
Run:
Operational advice:
- Consider staggering load vs post to reduce overlap.
- If transact is heavy, keep its concurrency lower unless the handler is idempotent and protected by dedupe/locks.
🏁 Script executed:
#!/bin/bash
# Verify struct tags/fields for Jobs in fees config
rg -n 'type .*Job|struct \{|\byaml:"cronexpr"|\byaml:"max_concurrent_jobs"' -A3
# Verify mapping from config.jobs.* to scheduler setup
rg -n 'startLoadingFees|startTransactingFees|startPostTx|cronexpr|max_concurrent_jobs' -A4
# Verify cron parser supports "@every"
rg -n '@every|cron\.New|WithSeconds|ParseStandard'Length of output: 22380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== go.mod ==="
sed -n '1,200p' go.mod || true
echo
echo "=== grep for robfig/cron in repo ==="
rg --hidden --no-ignore -n "robfig/cron" || true
echo
echo "=== cmd/fees/worker/main.go (full) ==="
sed -n '1,400p' cmd/fees/worker/main.go || true
echo
echo "=== plugin/fees/config.go (full) ==="
sed -n '1,260p' plugin/fees/config.go || true
echo
echo "=== search for MaxConcurrentJobs usage ==="
rg -n "MaxConcurrentJobs|max_concurrent_jobs" || true
echo
echo "=== search for asynq client/server usage ==="
rg -n "asynq.New|asynq.Client|asynq.Server|asynq.NewClient|asynq.NewServer" || true
echoLength of output: 26213
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== semaphore usages ==="
rg -n "semaphore.NewWeighted|semaphore" || true
echo
echo "=== plugin/fees/fees.go (full) ==="
sed -n '1,400p' plugin/fees/fees.go || true
echo
echo "=== plugin/fees/post_tx.go (full) ==="
sed -n '1,400p' plugin/fees/post_tx.go || true
echoLength of output: 14752
🌐 Web query:
Does robfig/cron v3 support the "@every" descriptor and 5-field cron specs (e.g. "0 12 * * 5")? cron.New() default parser behavior
💡 Result:
Short answer: Yes.
- @every (and the other descriptors like @daily/@hourly) are supported in v3. (pkg.go.dev)
- The default parser in v3 expects the standard 5-field cron spec (minute hour day-of-month month day-of-week), so "0 12 * * 5" is accepted by cron.New() without any special options. v3 intentionally switched the default to the standard 5-field form (the old v1/v2 behavior that included an initial seconds field is no longer the default). (pkg.go.dev, github.com)
If you need a seconds field (6-field/quartz-like) or want to customise parsing, construct the Cron with options, e.g.:
- Required seconds: cron.New(cron.WithSeconds())
- Custom parser: cron.New(cron.WithParser(cron.NewParser(...flags...))) (pkg.go.dev, github.com)
If you want, I can show a tiny code example demonstrating AddFunc with "@every 1h" and with "0 12 * * 5".
Citations:
- 1: https://pkg.go.dev/github.com/robfig/cron/v3?utm_source=chatgpt.com
- 2: https://pkg.go.dev/github.com/robfig/cron/v3?utm_source=chatgpt.com
- 3: https://github.com/robfig/cron?utm_source=chatgpt.com
- 4: https://pkg.go.dev/github.com/robfig/cron/v3?utm_source=chatgpt.com
- 5: https://github.com/robfig/cron?utm_source=chatgpt.com
Confirm cron keys & syntax; concurrency is worker-side (semaphores), not the enqueuer
Snippet (unchanged):
jobs:
load:
cronexpr: "@every 2m"
max_concurrent_jobs: 10
transact:
cronexpr: "0 12 * * 5"
max_concurrent_jobs: 10
post:
cronexpr: "@every 5m"
max_concurrent_jobs: 10
Summary (very short):
- Keys exist and are unmarshaled by viper (plugin/fees/config.go — Jobs struct + mapstructure tags; defaults set in withDefaults).
- robfig/cron v3 (present in go.mod) + cron.New() accept both "@every 2m" and "0 12 * * 5" (v3 default parser = standard 5-field, descriptors supported).
- MaxConcurrentJobs is used inside handlers to limit concurrent processing via semaphore.NewWeighted(...) (plugin/fees/fees.go — Load/Transact, plugin/fees/post_tx.go — Post). It does NOT throttle the scheduler/enqueuer.
- The scheduler simply calls asynqClient.Enqueue (cmd/fees/worker/main.go — cron.New().AddFunc(...)), and asynq server concurrency is hard-coded to 10 in cmd/fees/worker/main.go.
Files to review / attention:
- plugin/fees/config.go — Jobs struct, defaults and validation (cronexpr, max_concurrent_jobs).
- etc/vultisig/fee.yml — current cron exprs shown above.
- cmd/fees/worker/main.go — cron.New() + AddFunc uses feePluginConfig.Jobs..Cronexpr; asynq.NewServer configured with Concurrency: 10; start functions enqueue tasks.
- plugin/fees/fees.go & plugin/fees/post_tx.go — semaphore.NewWeighted(int64(fp.config.Jobs.*.MaxConcurrentJobs)) used to limit processing concurrency.
Actionable recommendation (short):
- If the intent was to throttle/enforce a single in-flight scheduled job (avoid over-enqueueing), add explicit checks before enqueueing (use asynq.Inspector to check queue / inflight tasks, or use a distributed lock/dedupe), or make asynq.Server concurrency configurable from FeeConfig and align it with MaxConcurrentJobs.
- Consider staggering load vs post or lowering transact concurrency if transact work is heavy or not idempotent.
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 16-16: no new line character at the end of file
(new-line-at-end-of-file)
🤖 Prompt for AI Agents
In etc/vultisig/fee.yml lines 7-16, the cron entries are valid but the
configured max_concurrent_jobs only gates worker-side processing (semaphores)
and does not prevent the scheduler/enqueuer from pushing many tasks; to fix this
either (A) add a pre-enqueue guard in cmd/fees/worker/main.go to avoid
over-enqueueing (e.g., use asynq.Inspector to inspect queue/inflight counts or
acquire a distributed lock/dedupe key before calling asynqClient.Enqueue), or
(B) make asynq.Server concurrency configurable from the fee config and align
asynq concurrency with Jobs.*.MaxConcurrentJobs so enqueue rate matches worker
capacity; optionally stagger schedules or lower transact concurrency if that job
is heavy.
| # Fees plugin | ||
|
|
||
| The fees pluigin is responsible for collecting user based fees incurred from other plugins |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Adopt singular "fee" naming and fix typo in intro
Aligns with the project's naming convention and fixes a spelling error.
Apply:
-# Fees plugin
+# Fee plugin
@@
-The fees pluigin is responsible for collecting user based fees incurred from other plugins
+The fee plugin is responsible for collecting user-based fees incurred from other pluginsAdditionally, consider renaming this file to FEE.md to fully comply with the “use fee (singular)” guideline.
📝 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.
| # Fees plugin | |
| The fees pluigin is responsible for collecting user based fees incurred from other plugins | |
| # Fee plugin | |
| The fee plugin is responsible for collecting user-based fees incurred from other plugins |
🤖 Prompt for AI Agents
In plugin/fees/FEES.md around lines 1 to 3, the intro uses plural "fees" and
contains a typo ("pluigin"); update the wording to use the singular "fee"
throughout and fix the spelling (e.g., "The fee plugin is responsible for
collecting user-based fees incurred from other plugins"), and rename the file to
FEE.md to match the project's singular naming convention.
| - *currently pending pr* - fee ids are passed along with a signing request which are used to verify that the amount being requested for a signature is the same. | ||
| - *current* - the system checks for all fees which are due to be collected and if the amount doesn't match it rejects a sign request. | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update Tx signing notes: remove “pending pr” and reflect current behavior
The PR introduces MarkFeeAsCollected and fee-id-aware signing. Remove “pending pr” and document the actual behavior.
-#### Tx signing
-
-- *currently pending pr* - fee ids are passed along with a signing request which are used to verify that the amount being requested for a signature is the same.
-- *current* - the system checks for all fees which are due to be collected and if the amount doesn't match it rejects a sign request.
+#### Tx signing
+
+- Fee IDs are included with the signing request and validated so the requested amount matches the sum of pending fees.
+- If the amount does not match, the sign request is rejected.📝 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.
| - *currently pending pr* - fee ids are passed along with a signing request which are used to verify that the amount being requested for a signature is the same. | |
| - *current* - the system checks for all fees which are due to be collected and if the amount doesn't match it rejects a sign request. | |
| #### Tx signing | |
| - Fee IDs are included with the signing request and validated so the requested amount matches the sum of pending fees. | |
| - If the amount does not match, the sign request is rejected. |
🤖 Prompt for AI Agents
In plugin/fees/FEES.md around lines 25 to 27, remove the phrase "currently
pending pr" and update the two bullet points to reflect the implemented
behavior: state that fee IDs are now passed with signing requests and used to
verify the requested signature amount matches the fee record (fee-id-aware
signing), and that the system checks all fees due and rejects sign requests when
amounts don't match; also mention the new MarkFeeAsCollected flow which marks
fees collected once verified. Keep the wording concise and replace the outdated
"pending pr" note with the concrete implemented behavior.
| **Trigger**: Scheduled cron job (configurable interval, default: every 10 minutes) | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Align schedule with current config defaults
Doc says “every 10 minutes” but fee.yml uses @every 2m per the PR summary. Adjust or explicitly reference config.
-**Trigger**: Scheduled cron job (configurable interval, default: every 10 minutes)
+**Trigger**: Scheduled cron job (configurable; default: @every 2m, see fee.yml)If the default has changed, please update fee.yml accordingly or confirm the intended default.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Repo root: $(pwd) ==="
echo
echo "1) Find files with 'fee' in the name (yml/yaml):"
find . -type f \( -iname "*fee*.yml" -o -iname "*fee*.yaml" \) -print || true
echo
echo "2) Ripgrep for '@every' and 'fee.yml' occurrences:"
rg --hidden -n --no-ignore-vcs -S '@every' || true
rg --hidden -n --no-ignore-vcs -S 'fee\.ya?ml' || true
echo
echo "3) Show content of likely fee config files (first 200 lines each):"
files=$(find . -type f \( -iname "*fee*.yml" -o -iname "*fee*.yaml" \) -print)
if [ -z "$files" ]; then
echo "No fee*.yml/yaml files found by name. Searching for 'fee' directories under plugin/fees..."
files=$(find plugin -type f -iname "*.yml" -o -iname "*.yaml" | rg -n 'fees|fee' || true)
fi
if [ -n "$files" ]; then
echo "$files" | while read -r f; do
echo
echo "---- $f ----"
sed -n '1,200p' "$f" || true
done
else
echo "No fee-related yaml files found to display."
fiLength of output: 2145
Align FEES.md schedule with actual config defaults
FEES.md states "every 10 minutes" but the repo config uses different defaults (load = "@every 2m", post = "@every 5m"). Update the docs or confirm/change the config.
Files to update / check:
- plugin/fees/FEES.md — lines ~48-49 (replace the "every 10 minutes" text).
- etc/vultisig/fee.yml — jobs.load.cronexpr: "@every 2m", jobs.post.cronexpr: "@every 5m", jobs.transact.cronexpr: "0 12 * * 5"
- plugin/fees/config.go — c.Jobs.Load.Cronexpr = "@every 2m" (line ~52), c.Jobs.Post.Cronexpr = "@every 5m" (line ~54)
Suggested change:
-**Trigger**: Scheduled cron job (configurable interval, default: every 10 minutes)
+**Trigger**: Scheduled cron job (configurable; defaults: load @every 2m, post @every 5m — see etc/vultisig/fee.yml)If the intended default is different, update etc/vultisig/fee.yml and plugin/fees/config.go to match, or confirm and keep the doc change.
📝 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.
| **Trigger**: Scheduled cron job (configurable interval, default: every 10 minutes) | |
| **Trigger**: Scheduled cron job (configurable; defaults: load @every 2m, post @every 5m — see etc/vultisig/fee.yml) |
🤖 Prompt for AI Agents
In plugin/fees/FEES.md around lines 48-49 the schedule text "every 10 minutes"
is incorrect; update that phrase to reflect the actual defaults used in the repo
(load = "@every 2m", post = "@every 5m") or change the config to match the doc.
Edit plugin/fees/FEES.md to replace "every 10 minutes" with the correct
schedules (e.g., "load: every 2 minutes; post: every 5 minutes"), and verify
consistency by checking etc/vultisig/fee.yml (jobs.load.cronexpr: "@every 2m",
jobs.post.cronexpr: "@every 5m", jobs.transact.cronexpr: "0 12 * * 5") and
plugin/fees/config.go (c.Jobs.Load.Cronexpr and c.Jobs.Post.Cronexpr) and update
either the documentation or the two config files so the doc and configs match.
| #### 2. Transaction Handling (`HandleTransactions`) | ||
|
|
||
| **Trigger**: Scheduled cron job (configurable interval, default: fridays weekly) | ||
|
|
||
| **Process**: | ||
| 1. Retrieves all fee runs in `draft` status | ||
| 2. For each valid fee run: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Document run-based transaction proposal and the intentionally unimplemented public method
Add an explicit note to prevent confusion for integrators.
-#### 2. Transaction Handling (`HandleTransactions`)
+#### 2. Transaction Handling (`HandleTransactions`)
+
+Note: The public `ProposeTransactions` method intentionally returns “not implemented” for the fee plugin v2. The actual logic uses a run-based flow implemented in the private `proposeTransactions(types.FeeRun)` method.
@@
-**Trigger**: Scheduled cron job (configurable interval, default: fridays weekly)
+**Trigger**: Scheduled cron job (configurable; default: 0 12 * 5, i.e. Fridays at 12:00 UTC)📝 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.
| #### 2. Transaction Handling (`HandleTransactions`) | |
| **Trigger**: Scheduled cron job (configurable interval, default: fridays weekly) | |
| **Process**: | |
| 1. Retrieves all fee runs in `draft` status | |
| 2. For each valid fee run: | |
| #### 2. Transaction Handling (`HandleTransactions`) | |
| Note: The public `ProposeTransactions` method intentionally returns “not implemented” for the fee plugin v2. The actual logic uses a run-based flow implemented in the private `proposeTransactions(types.FeeRun)` method. | |
| **Trigger**: Scheduled cron job (configurable; default: 0 12 * 5, i.e. Fridays at 12:00 UTC) | |
| **Process**: | |
| 1. Retrieves all fee runs in `draft` status | |
| 2. For each valid fee run: |
🤖 Prompt for AI Agents
In plugin/fees/FEES.md around lines 65 to 71, the docs for "Transaction Handling
(HandleTransactions)" lack an explicit note about the run-based transaction
proposal and an intentionally unimplemented public method; update this section
to add a clear, concise paragraph stating that transactions are handled per fee
run (describe the run-based approach in one sentence) and explicitly note which
public method is intentionally left unimplemented, why (e.g., internal-only /
planned future work), and what integrators should use instead (e.g., supported
public API or cron trigger). Ensure the note prevents confusion by naming the
method, stating its current status, and pointing to the recommended integration
path.
| **Trigger**: Scheduled cron job (configurable interval, default: every 10 minutes) | ||
|
|
||
| **Process**: |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Align post-tx schedule with current config defaults
Matches the fee.yml example in the PR summary.
-**Trigger**: Scheduled cron job (configurable interval, default: every 10 minutes)
+**Trigger**: Scheduled cron job (configurable; default: @every 5m, see fee.yml)Please confirm the intended default and update either the doc or config if they differ.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- plugin/fees/FEES.md (lines 1-200) ----"
if [ -f plugin/fees/FEES.md ]; then
sed -n '1,200p' plugin/fees/FEES.md
else
echo "plugin/fees/FEES.md not found"
fi
echo
echo "---- Searching repository for '@every', 'every 5m', 'every 10m', or '10 minutes' ----"
rg -n --hidden -S '@every|every\s*5m|every\s*10m|10\s*minutes' || true
echo
echo "---- Searching repository for fee.yml/fee.yaml/fees.yml/fees.yaml strings ----"
rg -n --hidden -S 'fee\.yml|fee\.yaml|fees\.yml|fees\.yaml' || true
echo
echo "---- Show plugin/fees/fee.yml if present ----"
if [ -f plugin/fees/fee.yml ]; then
sed -n '1,200p' plugin/fees/fee.yml
else
echo "plugin/fees/fee.yml not found"
fi
echo
echo "---- Show root fee.yml if present ----"
if [ -f fee.yml ]; then
sed -n '1,200p' fee.yml
else
echo "fee.yml not found at repo root"
fiLength of output: 9886
Align scheduling defaults in FEES.md with config values
FEES.md documents "default: every 10 minutes" for the LoadFees and HandlePostTx jobs, but the repository defaults are @every 2m (load) and @every 5m (post). Update the docs or the config so they match.
Files/locations to fix:
- plugin/fees/FEES.md — update Trigger lines at (rg matches) lines 48 and 86.
- plugin/fees/config.go — current values: Jobs.Load.Cronexpr = "@every 2m" (line ~52), Jobs.Post.Cronexpr = "@every 5m" (line ~54).
- etc/vultisig/fee.yml — cronexpr entries: "@every 2m" and "@every 5m" (lines ~9 and ~15).
- cmd/fees/worker/main.go — update stale comments referencing "every 10 minutes" (lines ~180 and ~194).
Suggested diffs for FEES.md:
-**Trigger**: Scheduled cron job (configurable interval, default: every 10 minutes)
+**Trigger**: Scheduled cron job (configurable; default: @every 2m — see plugin/fees/config.go or etc/vultisig/fee.yml)(for LoadFees)
-**Trigger**: Scheduled cron job (configurable interval, default: every 10 minutes)
+**Trigger**: Scheduled cron job (configurable; default: @every 5m — see plugin/fees/config.go or etc/vultisig/fee.yml)(for HandlePostTx)
Please update the docs or adjust the config if the intended defaults are different.
📝 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.
| **Trigger**: Scheduled cron job (configurable interval, default: every 10 minutes) | |
| **Process**: | |
| **Trigger**: Scheduled cron job (configurable; default: @every 5m — see plugin/fees/config.go or etc/vultisig/fee.yml) | |
| **Process**: |
| 1. **Resource Validation**: Only `ethereum.erc20.transfer` operations allowed | ||
| 2. **Recipient Validation**: Must use `VULTISIG_TREASURY` magic constant | ||
| 3. **Amount Constraints**: Maximum fee amount enforcement |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Restrict resource to USDC transfer per policy constraints
Project policy is USDC-only, not general ERC20.
-1. **Resource Validation**: Only `ethereum.erc20.transfer` operations allowed
+1. **Resource Validation**: Only `ethereum.usdc.transfer` operations allowedTo verify consistency across the repo:
🏁 Script executed:
#!/bin/bash
# Check occurrences of resource names and verify USDC-only constraint mentioned in policy/validation code and docs.
rg -n "ethereum\.erc20\.transfer|ethereum\.usdc\.transfer" -A 2Length of output: 1686
Restrict fees plugin to USDC transfers — change required
FEES.md and the fees plugin contain inconsistent resource names; the fees plugin must be USDC-only.
Files needing changes:
- plugin/fees/FEES.md (line ~112): replace
ethereum.erc20.transfer→ethereum.usdc.transfer - plugin/fees/policy.go (lines ~46-47): replace
Full: "ethereum.erc20.transfer"→Full: "ethereum.usdc.transfer" - plugin/fees/transaction.go (line ~73): replace
case "ethereum.erc20.transfer":→case "ethereum.usdc.transfer":
Note: plugin/payroll/transaction.go contains ethereum.erc20.transfer (lines ~314–316) — likely unrelated to the fees plugin; leave or review separately if intended.
Updated diff (docs):
-1. **Resource Validation**: Only `ethereum.erc20.transfer` operations allowed
+1. **Resource Validation**: Only `ethereum.usdc.transfer` operations allowed(Used retrieved learning confirming the fees plugin is USDC-only.)
📝 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.
| 1. **Resource Validation**: Only `ethereum.erc20.transfer` operations allowed | |
| 2. **Recipient Validation**: Must use `VULTISIG_TREASURY` magic constant | |
| 3. **Amount Constraints**: Maximum fee amount enforcement | |
| 1. **Resource Validation**: Only `ethereum.usdc.transfer` operations allowed | |
| 2. **Recipient Validation**: Must use `VULTISIG_TREASURY` magic constant | |
| 3. **Amount Constraints**: Maximum fee amount enforcement |
🤖 Prompt for AI Agents
In plugin/fees/FEES.md around lines 112–114, plugin/fees/policy.go around lines
46–47, and plugin/fees/transaction.go around line 73 the resource name
incorrectly uses "ethereum.erc20.transfer"; change each occurrence to
"ethereum.usdc.transfer" to restrict the fees plugin to USDC-only and keep docs,
policy, and transaction code consistent; do not modify
plugin/payroll/transaction.go (lines ~314–316) as that is separate.
| 1. **`GetPublicKeysFees(publicKey)`** - Retrieves pending fees for a vault | ||
| 2. **`MarkFeeAsCollected(txHash, timestamp, feeIds...)`** - Marks fees as collected | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Specify HTTP endpoints and payload for verifier integration
Make the API contract explicit and remove ambiguity around method names.
-1. **`GetPublicKeysFees(publicKey)`** - Retrieves pending fees for a vault
-2. **`MarkFeeAsCollected(txHash, timestamp, feeIds...)`** - Marks fees as collected
+1. GET `/fees?publicKey={publicKey}` - Retrieves pending fees for a vault
+2. POST `/fees/collected` - Marks fees as collected
+ Body (JSON):
+ {
+ "txHash": "0x...",
+ "collectedAt": "2025-08-01T12:34:56Z",
+ "feeIds": ["fee_123", "fee_456"]
+ }If the actual endpoints or parameter names differ, please update the doc to match the implementation. You can confirm with:
🏁 Script executed:
#!/bin/bash
# Discover the verifier client methods and routes
rg -n "MarkFeeAsCollected|/fees/collected|GetPublicKey|GetPublicKeysFees|/fees\?" -A 3Length of output: 1908
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,240p' internal/verifierapi/fees.goLength of output: 3044
Update FEES.md to match verifier API (endpoints & JSON payloads)
The verifier implements GET /fees/publickey/{ecdsaPublicKey} and POST /fees/collected with snake_case JSON keys — update the doc to match.
Files to update / reference:
- plugin/fees/FEES.md (lines ~128-130) — update these entries
- internal/verifierapi/fees.go — source of truth for endpoints/payload
- plugin/fees/fees.go and plugin/fees/post_tx.go — caller examples
Suggested diff:
-1. **`GetPublicKeysFees(publicKey)`** - Retrieves pending fees for a vault
-2. **`MarkFeeAsCollected(txHash, timestamp, feeIds...)`** - Marks fees as collected
+1. GET `/fees/publickey/{ecdsaPublicKey}` - Retrieves pending fees for a vault
+ Response: 200 JSON -> APIResponse with "data" containing FeeHistoryDto
+2. POST `/fees/collected` - Marks fees as collected
+ Body (JSON):
+ {
+ "ids": ["fee-uuid-1", "fee-uuid-2"],
+ "tx_hash": "0x...",
+ "collected_at": "2025-08-01T12:34:56Z" // ISO8601 timestamp
+ }
+ Response: 200 OK📝 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.
| 1. **`GetPublicKeysFees(publicKey)`** - Retrieves pending fees for a vault | |
| 2. **`MarkFeeAsCollected(txHash, timestamp, feeIds...)`** - Marks fees as collected | |
| 1. GET `/fees/publickey/{ecdsaPublicKey}` - Retrieves pending fees for a vault | |
| Response: 200 JSON -> APIResponse with "data" containing FeeHistoryDto | |
| 2. POST `/fees/collected` - Marks fees as collected | |
| Body (JSON): | |
| { | |
| "ids": ["fee-uuid-1", "fee-uuid-2"], | |
| "tx_hash": "0x...", | |
| "collected_at": "2025-08-01T12:34:56Z" // ISO8601 timestamp | |
| } | |
| Response: 200 OK |
🤖 Prompt for AI Agents
In plugin/fees/FEES.md around lines 128-130, the documented API methods are out
of sync with the verifier: replace the two list entries with the actual
endpoints and snake_case payloads used by the verifier — document GET
/fees/publickey/{ecdsaPublicKey} (returns pending fees for the vault) and POST
/fees/collected with a snake_case JSON body (e.g., tx_hash, timestamp, fee_ids
array) and the expected response; use internal/verifierapi/fees.go as the source
of truth and mirror the caller examples in plugin/fees/fees.go and
plugin/fees/post_tx.go to ensure endpoint paths and JSON key names match
exactly.
RaghavSood
left a comment
RaghavSood
left a comment
There was a problem hiding this comment.
LGTM for WIP
@garry-sharp Can we rebase once to make sure nothing else breaks before merging?
| "github.com/vultisig/plugin/storage/postgres" | ||
| ) | ||
|
|
||
| func startLoadingFees(asynqClient *asynq.Client, logger *logrus.Logger) { |
There was a problem hiding this comment.
This should probably have a mutex - it's possible that in theory the cron run could trigger while the previous run hasn't completed yet
There was a problem hiding this comment.
There is a mutex in the service itself which stops loads and txs happening synchronously
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores
Documentation