Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for fee runs within the plugin, including database schema, service/API extensions, and transaction proposal logic.
- Introduced
fee_runandfeetables (with view, indexes, and triggers) for tracking fee collections. - Extended storage and service layers to create and query fee runs (
CreateFeeRun) and filter active policies. - Implemented core
FeePluginworkflows for collecting fees, proposing keysign requests, and integrating with the verifier and transaction indexer.
Reviewed Changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| storage/postgres/policy.go | Added onlyActive flag to filter plugin policies on active state |
| storage/postgres/migrations/plugin/20250630152230_fee_runs.sql | Created fee_run and fee schema, view, indexes, and triggers |
| storage/postgres/fees.go | Implemented CreateFeeRun method with transaction logic |
| storage/db.go | Updated DatabaseStorage interface to include CreateFeeRun |
| service/policy.go | Updated service signature to propagate onlyActive |
| plugin/fees/transaction.go | Updated transaction builder to return PluginKeysignRequest |
| plugin/fees/fees.go | Extended plugin with collection flows and dependencies |
| plugin/common/nonce.go | Added NonceManager for nonce handling |
| internal/verifierapi/fees.go | Expanded DTOs and added endpoints for public key & all-fee queries |
| common/config.go / cmd/* / api/* | Refactored config loading and server initialization to common pkg |
Comments suppressed due to low confidence (2)
storage/postgres/fees.go:23
- Hardcoding the plugin ID string here can lead to typos; consider using a shared constant (e.g., PLUGIN_ID) to ensure consistency across the codebase.
if pluginId != "vultisig-fees-feee" {
scripts/dev/create_fee_policy/dummy_server.go:124
- Passing
nilfor thepluginparameter toNewServerwill likely cause a nil pointer inside the server; restore passing the instantiated plugin (e.g.,&p).
nil,
| Messages: []vtypes.KeysignMessage{ | ||
| { | ||
| TxIndexerID: "TODO", | ||
| Message: hex.EncodeToString([]byte{}), |
There was a problem hiding this comment.
Encoding an empty byte slice will always produce an empty message; you should encode the actual transaction bytes (e.g., hex.EncodeToString(txBytes)).
| Message: hex.EncodeToString([]byte{}), | |
| Message: hex.EncodeToString(txBytes), |
| url := fmt.Sprintf("/fees/publickey/%s", ecdsaPublicKey) | ||
| v.logger.Debug("Getting public key fees for public key: ", ecdsaPublicKey) | ||
| v.logger.Debug("URL: ", url) | ||
| response, err := v.get(url) |
There was a problem hiding this comment.
HTTP response bodies should be closed to avoid resource leaks. After reading or decoding response.Body, call defer response.Body.Close().
There was a problem hiding this comment.
Correct point by AI above (close response.Body)
|
|
||
| type NonceManager struct { | ||
| rpcClient *ethclient.Client | ||
| nonceMap sync.Map |
There was a problem hiding this comment.
The nonceMap field is declared but never used; consider removing it or implementing its intended functionality to prevent dead code.
| nonceMap sync.Map |
|
Warning Rate limit exceeded@garry-sharp has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 37 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughThis update introduces a comprehensive fee collection and management system, including new database tables, Go types, and plugin logic. It implements unsigned Ethereum (ERC20 and native) transaction generation, nonce management, policy and fee run tracking, and integration with vault and indexer services. Supporting configuration and migration scripts are added. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FeePlugin
participant VerifierAPI
participant VaultStorage
participant DB as Database
participant Indexer
participant Asynq
Client->>FeePlugin: Trigger HandleCollections
FeePlugin->>VerifierAPI: GetAllPublicKeysFees()
VerifierAPI-->>FeePlugin: Fee data per public key
loop For each public key with pending fees
FeePlugin->>DB: GetPluginPolicies(publicKey, onlyActive=true)
DB-->>FeePlugin: Policy list
FeePlugin->>DB: CreateFeeRun(policyID, fees)
DB-->>FeePlugin: FeeRun record
FeePlugin->>VaultStorage: GetVaultBackup(policyID)
VaultStorage-->>FeePlugin: Vault data
FeePlugin->>FeePlugin: ProposeTransactions(policy)
FeePlugin->>Indexer: Check for recent transaction
alt No recent transaction
FeePlugin->>FeePlugin: GenUnsignedTx(...)
FeePlugin->>Indexer: CreateTransaction(...)
FeePlugin->>Asynq: Enqueue signing task
Asynq-->>FeePlugin: Task result
FeePlugin->>Indexer: SigningComplete(...)
end
end
Possibly related PRs
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
♻️ Duplicate comments (5)
plugin/common/nonce.go (1)
14-14: Address the unused nonceMap field.The
nonceMapfield is declared and used inResetNonce()but never populated inGetNextNonce(), making the caching mechanism incomplete. Either implement proper nonce caching or remove the unused field.Consider one of these approaches:
Option 1: Implement proper caching
func (n *NonceManager) GetNextNonce(address string) (uint64, error) { n.mu.Lock() defer n.mu.Unlock() + // Check cache first + if cachedNonce, ok := n.nonceMap.Load(address); ok { + nonce := cachedNonce.(uint64) + n.nonceMap.Store(address, nonce+1) + return nonce, nil + } nonce, err := n.rpcClient.PendingNonceAt(context.Background(), common.HexToAddress(address)) if err != nil { return 0, fmt.Errorf("failed to get nonce from network: %w", err) } + n.nonceMap.Store(address, nonce+1) return nonce, nil }Option 2: Remove unused caching
type NonceManager struct { rpcClient *ethclient.Client - nonceMap sync.Map mu sync.Mutex } -func (n *NonceManager) ResetNonce(address string) { - n.nonceMap.Delete(address) -}internal/verifierapi/fees.go (2)
93-93: Close HTTP response body to avoid resource leaksHTTP response bodies should be closed after use to prevent resource leaks.
Add
defer response.Body.Close()after checking the error:response, err := v.get(url) if err != nil { v.logger.WithError(err).Error("failed to get public key fees") return FeeHistoryDto{}, fmt.Errorf("failed to get public key fees: %w", err) } +defer response.Body.Close()
127-127: Close HTTP response body to avoid resource leaksHTTP response bodies should be closed after use to prevent resource leaks.
Add
defer response.Body.Close()after checking the error:response, err := v.get(url) if err != nil { v.logger.WithError(err).Error("failed to get all public key fees") return map[string]FeeHistoryDto{}, fmt.Errorf("failed to get all public key fees: %w", err) } +defer response.Body.Close()plugin/fees/transaction.go (2)
64-64: The transaction is created but not assigned to a variable.The call to
etypes.NewTransactioncreates a transaction but doesn't capture its return value, making it unavailable for encoding.
64-81: The transaction needs to be properly created and encoded.The current implementation creates a transaction but doesn't use it, and encodes an empty byte slice instead of the actual transaction data.
Apply this diff to fix the transaction creation and encoding:
-etypes.NewTransaction(nonce, ethFromAddress, big.NewInt(0), uint64(fp.config.Gas.LimitMultiplier*ERC20_TRANSFER_GAS), gasPrice, data) +contractAddress := ecommon.HexToAddress(fp.config.CollectorAddress) +tx := etypes.NewTransaction(nonce, contractAddress, big.NewInt(0), uint64(fp.config.Gas.LimitMultiplier*ERC20_TRANSFER_GAS), gasPrice, data) + +txBytes, err := tx.MarshalBinary() +if err != nil { + return vtypes.PluginKeysignRequest{}, fmt.Errorf("failed to marshal transaction: %w", err) +} sessionID := uuid.New() return vtypes.PluginKeysignRequest{ KeysignRequest: vtypes.KeysignRequest{ PublicKey: vault.PublicKeyEcdsa, Messages: []vtypes.KeysignMessage{ { TxIndexerID: "TODO", - Message: hex.EncodeToString([]byte{}), + Message: hex.EncodeToString(txBytes), Chain: vcommon.Ethereum, }, }, SessionID: sessionID.String(), }, }, nil
🧹 Nitpick comments (10)
init-scripts/02_create_vultisig_verifier.sql (1)
1-1: Redundant, non-functional statement – consider deleting or converting to valid Postgres syntaxThe commented‐out
CREATE DATABASE IF NOT EXISTSis:
- Duplicated in
01_create_vultisig_plugin.sql, causing maintenance noise.- MySQL-style (
IF NOT EXISTS) and therefore invalid for Postgres, which this repo otherwise targets (see DSN in config).Either remove this file or replace the line with a Postgres-compatible snippet in the “up” migration that is actually executed.
create_buckets.sh (1)
8-12: Single-bucket variable will break existing consumers unless all plugins have migratedSwitching to a single bucket (
vultisig-plugin) is fine, but verify nothing still writes/reads the three legacy buckets.
It may be safer to keep an array for forward compatibility:-buckets="vultisig-plugin" +readonly buckets=("vultisig-plugin")Also consider
set -euo pipefailat the top so bucket creation failures surface in CI.init-scripts/01_create_vultisig_plugin.sql (1)
1-1: Commented SQL is dead code – remove or migrateAs with script 02, the line is inactive and uses MySQL syntax. Either:
- Drop it entirely and rely on IaC/helm to create the DB, or
- Add a real Postgres migration (e.g.
CREATE DATABASE vultisig_plugin;in an admin-level migration).Leaving commented DDL in version-controlled migrations is confusing during audits.
plugin/common/constants.go (1)
9-23: Missing golint-style comments for exported identifiers
EthereumMainnetEvmChainID,EthereumZeroAddressandErc20ABIare exported but lack doc comments that start with the identifier name, which will failgolint/staticcheckgates.-// ANY CONSTANTS THAT ARE USED IN MULTIPLE PLUGINS SHOULD BE DEFINED HERE. Like ERC20 ABI, zero addresses. +// EthereumMainnetEvmChainID is the chain-ID for Ethereum mainnet (1). +// EthereumZeroAddress represents the 0x…00 zero address. +// Erc20ABI is the minimal ABI fragment for ERC-20 `transfer`.config-plugin.yaml (1)
40-44: Inconsistent indentation & potential typo inlocal_party_prefix
- Other top-level sections use 2-space indents; this block uses 4, hurting readability.
vultisig-fees-feee(triple “e”) looks misspelled vs. constantvultisig-fees-feee? Confirm the exact plugin ID; mismatches will break party discovery.-vault_service: - relay: - server: https://api.vultisig.com/router - local_party_prefix: "vultisig-fees-feee" +vault_service: + relay: + server: https://api.vultisig.com/router + local_party_prefix: "vultisig-fees-fee"plugin/common/nonce.go (1)
24-33: Consider adding context parameter for better control.The method uses
context.Background()which doesn't allow callers to control timeouts or cancellation.-func (n *NonceManager) GetNextNonce(address string) (uint64, error) { +func (n *NonceManager) GetNextNonce(ctx context.Context, address string) (uint64, error) { n.mu.Lock() defer n.mu.Unlock() - nonce, err := n.rpcClient.PendingNonceAt(context.Background(), common.HexToAddress(address)) + nonce, err := n.rpcClient.PendingNonceAt(ctx, common.HexToAddress(address)) if err != nil { return 0, fmt.Errorf("failed to get nonce from network: %w", err) } return nonce, nil }storage/postgres/migrations/plugin/20250630152230_fee_runs.sql (1)
15-15: Missing UUID generation for fee.idThe
feetable'sidfield lacks a default UUID generation, unlike thefee_runtable. This requires the application to always provide UUIDs when inserting fees.Consider adding a default UUID generation for consistency:
- id UUID PRIMARY KEY, + id UUID PRIMARY KEY DEFAULT gen_random_uuid(),plugin/common/common.go (1)
90-90: Clarify nonce parameter usageThe
EvmEstimateTxfunction acceptsnonceas input and returns it unchanged as the first return value. This design is confusing and seems unnecessary.Consider either:
- Remove nonce from the return values since it's not modified
- Document why nonce needs to be passed through
func EvmEstimateTx( ctx context.Context, from, to gcommon.Address, value *big.Int, data []byte, rpcClient *ethclient.Client, nonce uint64, -) (uint64, uint64, *big.Int, *big.Int, gtypes.AccessList, error) { +) (uint64, *big.Int, *big.Int, gtypes.AccessList, error) {And update the return statement:
-return nonce, gasLimit, gasTipCap, maxFeePerGas, createAccessListRes.AccessList, nil +return gasLimit, gasTipCap, maxFeePerGas, createAccessListRes.AccessList, nilAlso applies to: 209-209
plugin/fees/transaction.go (2)
195-197: Fix error message prefix to use correct receiver name.The error message uses
p.genUnsignedTxbut should reference the actual function being called.Apply this diff:
-return []vtypes.PluginKeysignRequest{}, fmt.Errorf("p.genUnsignedTx: %w", e) +return []vtypes.PluginKeysignRequest{}, fmt.Errorf("plugincommon.GenUnsignedTx: %w", e)
210-212: Fix error message prefix to use correct receiver name.The error message prefix should use
fpinstead ofp.Apply this diff:
-return []vtypes.PluginKeysignRequest{}, fmt.Errorf("p.txIndexerService.CreateTx: %w", e) +return []vtypes.PluginKeysignRequest{}, fmt.Errorf("fp.txIndexerService.CreateTx: %w", e)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
api/plugin.go(1 hunks)api/server.go(3 hunks)api/server_config.go(0 hunks)cmd/dca/server/config.go(0 hunks)cmd/dca/server/main.go(2 hunks)cmd/fees/server/config.go(0 hunks)cmd/fees/server/main.go(3 hunks)cmd/fees/worker/main.go(5 hunks)cmd/payroll/server/config.go(1 hunks)common/config.go(3 hunks)config-plugin.yaml(1 hunks)create_buckets.sh(1 hunks)init-scripts/01_create_vultisig_plugin.sql(1 hunks)init-scripts/02_create_vultisig_verifier.sql(1 hunks)internal/types/fees.go(1 hunks)internal/verifierapi/fees.go(2 hunks)plugin/common/common.go(1 hunks)plugin/common/constants.go(1 hunks)plugin/common/nonce.go(1 hunks)plugin/fees/config.go(1 hunks)plugin/fees/constraints.go(1 hunks)plugin/fees/fees.go(8 hunks)plugin/fees/transaction.go(1 hunks)scripts/dev/create_fee_policy/dummy_server.go(3 hunks)scripts/dev/create_fee_policy/main.go(2 hunks)service/policy.go(2 hunks)storage/db.go(2 hunks)storage/postgres/fees.go(1 hunks)storage/postgres/migrations/plugin/20250630152230_fee_runs.sql(1 hunks)storage/postgres/policy.go(2 hunks)
💤 Files with no reviewable changes (3)
- api/server_config.go
- cmd/fees/server/config.go
- cmd/dca/server/config.go
🧰 Additional context used
🧠 Learnings (20)
plugin/fees/config.go (1)
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.
init-scripts/02_create_vultisig_verifier.sql (1)
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.
plugin/fees/constraints.go (3)
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.
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.
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.
config-plugin.yaml (2)
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.
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.
scripts/dev/create_fee_policy/dummy_server.go (4)
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.
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.
Learnt from: johnnyluo
PR: vultisig/plugin#108
File: Dockerfile.Payroll.server:14-19
Timestamp: 2025-07-02T04:55:36.331Z
Learning: In the vultisig/plugin repository, the team maintains both the main repository and the go-wrappers dependency repository, so they are comfortable downloading from the master branch rather than pinning to specific commits.
Learnt from: webpiratt
PR: 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.
cmd/dca/server/main.go (5)
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.
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.
Learnt from: johnnyluo
PR: vultisig/plugin#108
File: Dockerfile.Payroll.server:14-19
Timestamp: 2025-07-02T04:55:36.331Z
Learning: In the vultisig/plugin repository, the team maintains both the main repository and the go-wrappers dependency repository, so they are comfortable downloading from the master branch rather than pinning to specific commits.
Learnt from: 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.
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.
api/server.go (4)
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.
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.
Learnt from: johnnyluo
PR: vultisig/plugin#108
File: Dockerfile.Payroll.server:14-19
Timestamp: 2025-07-02T04:55:36.331Z
Learning: In the vultisig/plugin repository, the team maintains both the main repository and the go-wrappers dependency repository, so they are comfortable downloading from the master branch rather than pinning to specific commits.
Learnt from: 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.
api/plugin.go (1)
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.
cmd/fees/worker/main.go (4)
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.
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.
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.
Learnt from: webpiratt
PR: vultisig/plugin#105
File: plugin/dca/dca.go:734-736
Timestamp: 2025-07-01T17:35:35.277Z
Learning: The DCA plugin in the vultisig/plugin codebase is currently not fully implemented. Methods like getCompletedSwapTransactionsCount() return placeholder values (e.g., 0) with TODO comments, which is expected behavior during the development phase. The plugin implementation is intentionally incomplete while other parts of the system are being refactored.
cmd/payroll/server/config.go (5)
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.
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.
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.
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.
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.
cmd/fees/server/main.go (6)
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.
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.
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.
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.
Learnt from: johnnyluo
PR: vultisig/plugin#108
File: Dockerfile.Payroll.server:14-19
Timestamp: 2025-07-02T04:55:36.331Z
Learning: In the vultisig/plugin repository, the team maintains both the main repository and the go-wrappers dependency repository, so they are comfortable downloading from the master branch rather than pinning to specific commits.
Learnt from: webpiratt
PR: vultisig/plugin#105
File: plugin/dca/dca.go:734-736
Timestamp: 2025-07-01T17:35:35.277Z
Learning: The DCA plugin in the vultisig/plugin codebase is currently not fully implemented. Methods like getCompletedSwapTransactionsCount() return placeholder values (e.g., 0) with TODO comments, which is expected behavior during the development phase. The plugin implementation is intentionally incomplete while other parts of the system are being refactored.
storage/db.go (2)
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.
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.
storage/postgres/migrations/plugin/20250630152230_fee_runs.sql (2)
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)`.
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.
plugin/common/constants.go (4)
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.
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.
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.
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.
common/config.go (3)
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.
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.
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.
storage/postgres/fees.go (1)
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.
plugin/common/common.go (3)
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.
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.
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.
internal/verifierapi/fees.go (1)
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.
plugin/fees/fees.go (4)
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.
Learnt from: johnnyluo
PR: vultisig/plugin#108
File: Dockerfile.Payroll.server:14-19
Timestamp: 2025-07-02T04:55:36.331Z
Learning: In the vultisig/plugin repository, the team maintains both the main repository and the go-wrappers dependency repository, so they are comfortable downloading from the master branch rather than pinning to specific commits.
Learnt from: 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.
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.
plugin/fees/transaction.go (3)
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.
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.
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.
🧬 Code Graph Analysis (10)
scripts/dev/create_fee_policy/dummy_server.go (2)
api/server.go (1)
Server(31-44)common/config.go (1)
ServerConfig(14-20)
cmd/dca/server/main.go (1)
common/config.go (2)
LoadConfig(37-50)GetConfig(71-73)
api/server.go (1)
common/config.go (1)
ServerConfig(14-20)
storage/postgres/policy.go (1)
storage/postgres/db.go (1)
PostgresBackend(15-17)
cmd/payroll/server/config.go (1)
common/config.go (1)
ServerConfig(14-20)
storage/db.go (2)
internal/types/fees.go (2)
FeeRunState(11-11)FeeRun(27-36)internal/verifierapi/fees.go (1)
FeeDto(37-46)
common/config.go (3)
api/server.go (1)
Server(31-44)scripts/dev/create_fee_policy/dummy_server.go (1)
ReadConfig(48-64)cmd/payroll/server/config.go (1)
ReadConfig(38-55)
storage/postgres/fees.go (3)
storage/postgres/db.go (1)
PostgresBackend(15-17)internal/types/fees.go (2)
FeeRunState(11-11)FeeRun(27-36)internal/verifierapi/fees.go (1)
FeeDto(37-46)
plugin/common/common.go (1)
plugin/common/constants.go (3)
EthereumMainnetEvmChainID(11-11)EthereumZeroAddress(12-12)Erc20ABI(14-22)
internal/verifierapi/fees.go (1)
internal/verifierapi/verifierapi.go (2)
VerifierApi(27-30)APIResponse(12-18)
🪛 GitHub Check: build
plugin/fees/fees.go
[failure] 396-396:
fp.initSign undefined (type *FeePlugin has no field or method initSign)
[failure] 392-392:
undefined: errgroup
plugin/fees/transaction.go
[failure] 179-179:
undefined: p
🪛 golangci-lint (1.64.8)
plugin/fees/fees.go
392-392: undefined: errgroup
(typecheck)
396-396: fp.initSign undefined (type *FeePlugin has no field or method initSign)
(typecheck)
plugin/fees/transaction.go
179-179: undefined: p
(typecheck)
🪛 GitHub Actions: Go
plugin/fees/transaction.go
[error] 179-179: Build failed: undefined identifier 'p' at line 179.
🔇 Additional comments (29)
plugin/fees/config.go (1)
28-28: Good documentation improvement.The clarifying comment helps distinguish between plugin-specific and system-wide configuration, which is valuable given the broader configuration refactoring mentioned in the PR.
api/plugin.go (1)
161-161: GetPluginPolicies signature correctly includesonlyActiveparameterVerified that in service/policy.go (line 128) the method is defined as:
func (s *PolicyService) GetPluginPolicies(ctx context.Context, pluginID vtypes.PluginID, publicKey string, onlyActive bool) ([]vtypes.PluginPolicy, error) { … }No further changes are needed.
scripts/dev/create_fee_policy/dummy_server.go (2)
13-13: Configuration refactoring looks good.The import addition and struct field type change from
api.ServerConfigtocommon.ServerConfigare consistent with the broader configuration centralization effort mentioned in the PR.Also applies to: 24-24
124-124: Ensure passing nil for the plugin won’t cause a nil-pointer panicBefore approving the change in
scripts/dev/create_fee_policy/dummy_server.go(line 124) where you replace&pwithnil, please verify that no plugin methods will be invoked on the resulting server. In particular:• scripts/dev/create_fee_policy/dummy_server.go: Server initialized with
nilplugin (around line 124).
• api/plugin.go: Handlers call
– s.plugin.ValidatePluginPolicy(...)
– s.plugin.ValidateProposedTransactions(...)
– s.plugin.GetRecipeSpecification()
Any of these will panic ifs.pluginis nil.Confirm that your dummy server does not register or invoke any of these plugin endpoints (or that it re-initializes
s.pluginbefore use). If plugin routes are unused in this script, the change is safe; otherwise, inject a no-op plugin or restore&p.cmd/payroll/server/config.go (1)
11-11: Configuration refactoring is consistent.The import change and struct field type update from
api.ServerConfigtocommon.ServerConfigalign with the centralized configuration management approach. This is consistent with similar changes across the codebase.Also applies to: 16-16
api/server.go (2)
22-22: LGTM: Configuration centralization import.The addition of the common package import aligns with the broader refactoring to centralize configuration management across the codebase.
32-32: LGTM: Consistent configuration type refactoring.The change from local
ServerConfigtocommon.ServerConfigis consistent and well-executed. The Server struct field and NewServer constructor parameter both use the centralized type, maintaining consistency across the API layer.Also applies to: 48-48
cmd/dca/server/main.go (2)
16-16: LGTM: Configuration centralization import.The common package import supports the centralized configuration loading pattern used throughout the codebase.
25-29: LGTM: Consistent configuration loading pattern.The refactoring from local
GetConfigure()tocommon.LoadConfig()+common.GetConfig()follows the same pattern used in other server main files, improving code consistency and reducing duplication.storage/postgres/policy.go (2)
41-41: LGTM: Method signature updated for policy filtering.The addition of the
onlyActiveboolean parameter enables filtering of plugin policies by their active status, which aligns with the broader API changes for policy management.
53-55: LGTM: Safe and correct SQL query filtering.The conditional SQL query modification is implemented correctly:
- Uses proper conditional logic to append the filter
- Appends a static string, avoiding SQL injection risks
- Maintains the existing query structure when filtering is not needed
scripts/dev/create_fee_policy/main.go (2)
106-115: LGTM: Appropriate amount constraint for fee collection.The addition of a maximum amount constraint of 500000000 (500 USDC) is reasonable for fee collection policies and helps prevent excessive fee collection.
147-147: LGTM: Simplified server creation.The removal of vault storage logic and simplification of the dummy server creation aligns with the decoupling of vault insertion from policy creation, improving the script's focus.
storage/db.go (3)
12-12: LGTM: Import added for fee management types.The verifierapi package import supports the new
CreateFeeRunmethod's parameter types, particularly theFeeDtotype used in the method signature.
19-19: LGTM: Interface method updated for policy filtering.The addition of the
onlyActiveparameter toGetAllPluginPoliciesis consistent with the implementation changes instorage/postgres/policy.goand enables filtering of active plugin policies.
33-33: LGTM: New interface method for fee run management.The
CreateFeeRunmethod signature is well-designed with:
- Proper context handling for cancellation/timeout
- UUID parameter types for policy identification
- Custom types for fee run state and fee DTOs
- Appropriate return types (FeeRun object and error)
cmd/fees/server/main.go (2)
25-29: LGTM! Configuration refactoring looks good.The migration from local configuration loading to the centralized
commonpackage approach improves consistency across the codebase.
86-87: Good documentation of intentional parameter choice.The comment clearly explains why
txIndexerServiceis passed asnilfor the server, which helps future maintainers understand the design decision.internal/types/fees.go (2)
11-16: LGTM! Well-defined state constants.The
FeeRunStatetype with explicit constants provides type safety and clarity for fee run states.
19-24: LGTM! Clean fee record structure.The
Feestruct is well-designed with appropriate field types and database tags.service/policy.go (2)
20-20: LGTM! Interface updated consistently.The addition of the
onlyActiveparameter to the interface is properly reflected in the method signature.
128-129: LGTM! Implementation matches interface.The method implementation correctly accepts and passes through the new
onlyActiveparameter to the database layer.cmd/fees/worker/main.go (4)
25-29: LGTM! Consistent configuration refactoring.The migration to centralized configuration management aligns with similar changes in the server component.
48-48: LGTM! Added inspector for enhanced task management.The addition of
asynqInspectorprovides task inspection capabilities alongside the existing client.
91-91: LGTM! Enhanced fee plugin with additional dependencies.The fee plugin constructor now receives the necessary dependencies for vault storage, transaction indexing, and async task handling.
115-115: LGTM! Updated to use new fee collection type.The change from
FeeCollectionTypeByPolicytoFeeCollectionTypeAllreflects the expanded fee collection logic.internal/verifierapi/fees.go (1)
49-49: Double-check Removal of PolicyId in FeeHistoryDtoI wasn’t able to find any references to
PolicyIdin the Go codebase or JSON tags. Before permanently dropping this field, please verify there are no internal or external consumers (API clients, docs, integrations) relying onpolicy_id.• File impacted:
- internal/verifierapi/fees.go —
FeeHistoryDtonow comments out thePolicyIdfield- // PolicyId uuid.UUID `json:"policy_id" validate:"required"`• No other Go files reference
FeeHistoryDto.PolicyId(struct usages, marshaling, tests)
• Check API documentation or client code forpolicy_idexpectationsPlease confirm this change won’t break existing functionality.
plugin/common/common.go (1)
1-240: Well-structured Ethereum transaction utilitiesThe implementation provides a clean abstraction for creating unsigned Ethereum transactions with proper:
- Chain-specific routing architecture
- Distinction between native ETH and ERC20 transfers
- Concurrent gas estimation for performance
- EIP-1559 dynamic fee transaction support
- Comprehensive error handling
plugin/fees/fees.go (1)
107-107: Confirm the fee plugin ID constantI wasn’t able to locate
vtypes.PluginVultisigFees_feeein the codebase or verify it against the externalvtypespackage. Please double-check that:
- The constant
PluginVultisigFees_feeeis spelled correctly (no extra “e”s or typos).- It is defined in the
vtypespackage and is the intended ID for this fees plugin.- Its naming aligns with other plugin ID conventions in the project.
| var EthereumMainnetEvmChainID *big.Int = big.NewInt(1) // 1 is the chain id for Ethereum mainnet | ||
| var EthereumZeroAddress gcommon.Address = gcommon.HexToAddress("0x0000000000000000000000000000000000000000") |
There was a problem hiding this comment.
*Exporting a mutable big.Int opens you to accidental mutation
EthereumMainnetEvmChainID is a pointer; callers can mutate it:
EthereumMainnetEvmChainID.SetInt64(1337) // Oops – global state changedSafer patterns:
-var EthereumMainnetEvmChainID *big.Int = big.NewInt(1)
+func EthereumMainnetEvmChainID() *big.Int { return big.NewInt(1) }or keep the var unexported and expose a getter.
Same caution applies to EthereumZeroAddress although common.Address is immutable.
🤖 Prompt for AI Agents
In plugin/common/constants.go at lines 11-12, the exported variable
EthereumMainnetEvmChainID is a pointer to a mutable big.Int, allowing callers to
accidentally mutate the global state. To fix this, make
EthereumMainnetEvmChainID unexported (lowercase) and provide a getter function
that returns a copy of the big.Int value, preventing external mutation.
Similarly, consider making EthereumZeroAddress unexported and expose it via a
getter to maintain immutability.
| data, err := erc20ABI.Pack("transfer", fp.config.CollectorAddress) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to pack erc20 abi: %w", err) | ||
| return vtypes.PluginKeysignRequest{}, fmt.Errorf("failed to pack erc20 abi: %w", err) | ||
| } |
There was a problem hiding this comment.
The transfer call is missing the amount parameter.
The ERC20 transfer function requires two parameters: recipient address and amount. Currently only the recipient is provided.
Apply this diff to fix the transfer call:
-data, err := erc20ABI.Pack("transfer", fp.config.CollectorAddress)
+data, err := erc20ABI.Pack("transfer", fp.config.CollectorAddress, big.NewInt(int64(amount)))Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In plugin/fees/transaction.go around lines 52 to 55, the ERC20 transfer call is
missing the amount parameter, which is required along with the recipient
address. Update the erc20ABI.Pack call to include both the recipient address and
the amount to transfer as arguments, ensuring the function signature matches the
ERC20 transfer method.
3601bc1 to
722615b
Compare
…Modified GetAllPluginPolicies to include an active status parameter, improved fee collection logic to handle fees more efficiently, and updated database queries for consistency.
webpiratt
left a comment
webpiratt
left a comment
There was a problem hiding this comment.
Looks good, I'll mark approved, I left few comments - feel free to change something in this PR or some of them in next PRs - it's up to you, it's not final Fee PR anyways
| url := fmt.Sprintf("/fees/publickey/%s", ecdsaPublicKey) | ||
| v.logger.Debug("Getting public key fees for public key: ", ecdsaPublicKey) | ||
| v.logger.Debug("URL: ", url) | ||
| response, err := v.get(url) |
There was a problem hiding this comment.
Correct point by AI above (close response.Body)
| } | ||
| } | ||
|
|
||
| func EvmMakeUnsignedTransfer( |
There was a problem hiding this comment.
These functions are correct, but feel free to replace it with SDK in some next PRs
|
|
||
| // ANY CONSTANTS THAT ARE USED IN MULTIPLE PLUGINS SHOULD BE DEFINED HERE. Like ERC20 ABI, zero addresses. | ||
|
|
||
| var EthereumMainnetEvmChainID *big.Int = big.NewInt(1) // 1 is the chain id for Ethereum mainnet |
There was a problem hiding this comment.
Just FYI: we have Ethereum evmID in verifier here - https://github.com/vultisig/verifier/blob/main/common/chain.go#L112
| rtypes.ScheduleFrequency_SCHEDULE_FREQUENCY_BIWEEKLY, | ||
| rtypes.ScheduleFrequency_SCHEDULE_FREQUENCY_MONTHLY, | ||
| }, | ||
| MaxScheduledExecutions: 100, //TODO garry - don't know if this is a good number or not |
There was a problem hiding this comment.
I also saw this parameter in other Recipe schemas, and if I understood it correctly it defines max times policy executed, and I think everywhere in should be 0 or -1 and handled as endless, otherwise it seems policy need to be recreated after some particular execution count reached
| PublicKey: vault.PublicKeyEcdsa, | ||
| Messages: []vtypes.KeysignMessage{ | ||
| { | ||
| TxIndexerID: "TODO", |
There was a problem hiding this comment.
Leave it nil or empty string if not used, so VaultManagementService could skip it correctly
| gasPrice = gasPrice.Mul(gasPrice, big.NewInt(int64(fp.config.Gas.PriceMultiplier))) | ||
|
|
||
| tx := types.NewTransaction(nonce, ethFromAddress, big.NewInt(0), uint64(fp.config.Gas.LimitMultiplier*ERC20_TRANSFER_GAS), gasPrice, data) | ||
| etypes.NewTransaction(nonce, ethFromAddress, big.NewInt(0), uint64(fp.config.Gas.LimitMultiplier*ERC20_TRANSFER_GAS), gasPrice, data) |
There was a problem hiding this comment.
I see that code with multiplier stuff copypasted from old plugin code (I remember similar lines), even if it works, it would overpay for gas or pay less than required in case of Ethereum network congestion (tx would stuck for a while). Please also refactor with sdk.MakeTx and codegenerated erc20.NewErc20().PackTransfer in this PR or next PRs
|
@garry-sharp migration CI test fails |
|
|
||
| mc alias set local http://localhost:9000 minioadmin minioadmin | ||
|
|
||
| buckets="vultisig-payroll vultisig-dca vultisig-fee" |
There was a problem hiding this comment.
This was a merge conflict on the rebasing which must of slipped through, restored
| @@ -1 +1 @@ | |||
| CREATE DATABASE "vultisig-payroll"; No newline at end of file | |||
| -- CREATE DATABASE IF NOT EXISTS "vultisig-plugin"; No newline at end of file | |||
There was a problem hiding this comment.
this is going to break the local development stack
|
|
||
| if response.StatusCode != http.StatusOK { | ||
| v.logger.WithError(err).Error("failed to get public key fees") | ||
| fmt.Println(response.StatusCode) |
There was a problem hiding this comment.
We have v.logger , let's keep using it , don't use fmt.Println , please clean up a few other places as well.
This PR is not in good quality , should not be approved.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
storage/postgres/schema/schema.sql (1)
1-242: Address the pipeline failure for schema synchronization.The migration CI test is failing because the schema.sql file is out of sync with the actual database migrations.
Please run the following command to update the schema file:
make dump-schema CONFIG=config-plugin.yamlThis will ensure the schema.sql file matches the current state of all applied migrations.
🧹 Nitpick comments (1)
plugin/fees/transaction.go (1)
124-124: Fix error message format string inconsistency.The error message format string references "p." but should reference "fp." to match the actual variable usage.
-return []vtypes.PluginKeysignRequest{}, fmt.Errorf("p.nonceManager.GetNextNonce: %w", err) +return []vtypes.PluginKeysignRequest{}, fmt.Errorf("fp.nonceManager.GetNextNonce: %w", err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
cmd/dca/server/config.go(4 hunks)cmd/dca/server/main.go(1 hunks)cmd/fees/server/main.go(1 hunks)cmd/fees/worker/config.go(1 hunks)cmd/fees/worker/main.go(2 hunks)cmd/payroll/server/config.go(3 hunks)docker-compose.yaml(1 hunks)init-scripts/02_create_vultisig_verifier.sql(1 hunks)plugin/common/common.go(1 hunks)plugin/common/nonce.go(1 hunks)plugin/fees/fees.go(8 hunks)plugin/fees/transaction.go(1 hunks)scripts/dev/create_fee_policy/dummy_server.go(1 hunks)storage/postgres/migrations/plugin/20250630152230_fee_runs.sql(1 hunks)storage/postgres/schema/schema.sql(5 hunks)
✅ Files skipped from review due to trivial changes (4)
- cmd/fees/worker/config.go
- cmd/dca/server/main.go
- docker-compose.yaml
- cmd/dca/server/config.go
🚧 Files skipped from review as they are similar to previous changes (6)
- scripts/dev/create_fee_policy/dummy_server.go
- init-scripts/02_create_vultisig_verifier.sql
- cmd/fees/server/main.go
- cmd/payroll/server/config.go
- storage/postgres/migrations/plugin/20250630152230_fee_runs.sql
- cmd/fees/worker/main.go
🧰 Additional context used
🧠 Learnings (4)
plugin/fees/transaction.go (5)
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.
Learnt from: webpiratt
PR: vultisig/plugin#105
File: plugin/dca/dca.go:734-736
Timestamp: 2025-07-01T17:35:35.277Z
Learning: The DCA plugin in the vultisig/plugin codebase is currently not fully implemented. Methods like getCompletedSwapTransactionsCount() return placeholder values (e.g., 0) with TODO comments, which is expected behavior during the development phase. The plugin implementation is intentionally incomplete while other parts of the system are being refactored.
Learnt from: webpiratt
PR: 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.
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.
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.
plugin/common/common.go (3)
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.
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.
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.
storage/postgres/schema/schema.sql (2)
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)`.
Learnt from: johnnyluo
PR: vultisig/plugin#82
File: internal/types/time_trigger.go:22-22
Timestamp: 2025-06-03T23:38:43.733Z
Learning: In the vultisig/plugin codebase, enum types like rtypes.ScheduleFrequency are stored as integers (INT) in the PostgreSQL database rather than as text strings, providing better type safety and performance.
plugin/fees/fees.go (6)
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.
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.
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.
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.
Learnt from: johnnyluo
PR: vultisig/plugin#108
File: Dockerfile.Payroll.server:14-19
Timestamp: 2025-07-02T04:55:36.331Z
Learning: In the vultisig/plugin repository, the team maintains both the main repository and the go-wrappers dependency repository, so they are comfortable downloading from the master branch rather than pinning to specific commits.
Learnt from: 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.
🧬 Code Graph Analysis (1)
plugin/common/common.go (1)
plugin/common/constants.go (1)
Erc20ABI(14-22)
🪛 golangci-lint (1.64.8)
plugin/common/common.go
1-1: : # github.com/vultisig/plugin/plugin/common
plugin/common/common.go:35:4: undefined: ethereumMainnetEvmChainID
plugin/common/common.go:71:16: undefined: ethereumZeroAddress
(typecheck)
plugin/fees/fees.go
23-23: could not import github.com/vultisig/plugin/plugin/common (-: # github.com/vultisig/plugin/plugin/common
plugin/common/common.go:35:4: undefined: ethereumMainnetEvmChainID
plugin/common/common.go:71:16: undefined: ethereumZeroAddress)
(typecheck)
🪛 GitHub Actions: Migration Test
storage/postgres/schema/schema.sql
[error] 1-1: Schema has changed but schema.sql was not updated! Please run 'make dump-schema CONFIG=config-plugin.yaml' to update the schema file.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (14)
plugin/common/nonce.go (1)
14-35: LGTM! Clean and thread-safe nonce management implementation.The NonceManager implementation is well-designed with proper synchronization using a mutex and clear error handling. The direct network queries without caching provide the most up-to-date nonce values, which is appropriate for fee collection transactions.
plugin/common/common.go (1)
120-210: Well-implemented concurrent gas estimation.The concurrent gas estimation using errgroup is well-designed, efficiently fetching gas limit, tip cap, and base fee in parallel. The access list creation via RPC call adds optimization for EIP-2930 transactions.
storage/postgres/schema/schema.sql (2)
36-43: Well-designed trigger function for automatic timestamp updates.The
update_updated_at_column()trigger function follows PostgreSQL best practices for automatically maintaining updated_at timestamps.
49-82: Excellent database design for fee tracking.The fee and fee_run table structure with the aggregating view provides a clean separation of concerns and efficient querying capabilities. The foreign key relationships and indexes are well-designed for the expected access patterns.
plugin/fees/transaction.go (5)
29-56: Solid vault integration and address derivation.The implementation properly retrieves the vault from policy using the encryption secret and derives the Ethereum address correctly. Good error handling throughout.
59-89: Proper policy validation and rule processing.The code correctly validates that only "allow-usdc-transfer-to-collector" rules are supported and extracts the recipient address from fixed value constraints. Good defensive programming with explicit error handling for unsupported rules.
98-121: Smart duplicate transaction prevention.The 6-hour time window check prevents duplicate fee collection transactions while allowing reasonable retry intervals. This balances efficiency with robustness.
127-183: Excellent transaction creation and signing workflow.The integration with the common transaction utilities and the structured keysign request creation is well-implemented. The transaction indexer integration provides good tracking capabilities.
226-290: Robust asynchronous signing implementation.The task-based signing with proper timeout, retry settings, and polling provides a solid foundation for handling signing workflows. Error handling and completion logic are comprehensive.
plugin/fees/fees.go (5)
52-65: Well-structured plugin with comprehensive dependencies.The FeePlugin struct properly includes all necessary dependencies for fee collection, transaction management, and vault operations. Good separation of concerns.
68-104: Robust constructor with proper validation.The constructor properly validates all dependencies and initializes the plugin with appropriate error handling. The nonce manager initialization and verifier API setup are well-integrated.
106-145: Comprehensive recipe specification.The recipe specification properly defines the plugin's capabilities, supported resources (ethereum.erc20.transfer), and parameter constraints. This follows the recipe schema format correctly.
296-334: Efficient fee collection implementation.The
collectAllFeesmethod efficiently processes all public keys with pending fees and validates fee amounts before executing collection. Good logging and error handling throughout.
336-400: Excellent fee collection execution workflow.The
executeFeeCollectionmethod properly orchestrates the entire fee collection process: policy retrieval, fee run creation, vault access, transaction proposal, and concurrent signing. The use of errgroup for concurrent signing is appropriate.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
plugin/fees/transaction.go (3)
107-107: Clarify the token identifier usage.The TODO comment suggests uncertainty about whether to use symbol or address, but
token.Addressis correct for ERC20 token identification in transaction indexing.- token.Address, //TODO check if this is symbol or address + token.Address,
124-124: Update error message prefixes for consistency.The error messages still use the old "p." prefix instead of "fp." for consistency with the receiver naming.
- return []vtypes.PluginKeysignRequest{}, fmt.Errorf("p.nonceManager.GetNextNonce: %w", err) + return []vtypes.PluginKeysignRequest{}, fmt.Errorf("fp.nonceManager.GetNextNonce: %w", err)- return []vtypes.PluginKeysignRequest{}, fmt.Errorf("p.genUnsignedTx: %w", e) + return []vtypes.PluginKeysignRequest{}, fmt.Errorf("fp.genUnsignedTx: %w", e)- return []vtypes.PluginKeysignRequest{}, fmt.Errorf("p.txIndexerService.CreateTx: %w", e) + return []vtypes.PluginKeysignRequest{}, fmt.Errorf("fp.txIndexerService.CreateTx: %w", e)Also applies to: 139-139, 154-154
223-223: Update error message prefix for consistency.The error message uses the old "p." prefix instead of "fp.".
- return false, fmt.Errorf("p.txIndexerService.GetTxInTimeRange: %w", err) + return false, fmt.Errorf("fp.txIndexerService.GetTxInTimeRange: %w", err)plugin/fees/fees.go (1)
367-378: Redundant vault retrieval.The vault is retrieved here but not used, as
ProposeTransactionsretrieves the vault again using the same logic. Consider removing this duplicate retrieval for efficiency.- //Get vault and check it exists - - vaultFileName := vcommon.GetVaultBackupFilename(ecdsaPublicKey, vtypes.PluginVultisigFees_feee.String()) - vaultContent, err := fp.vaultStorage.GetVault(vaultFileName) - if err != nil { - // TODO some real error handling here - fp.Log(logrus.DebugLevel, "Failed to get vault: ", err) - return fmt.Errorf("failed to get vault: %w", err) - } - if vaultContent == nil { - // TODO some real error handling here - fp.Log(logrus.DebugLevel, "Vault not found") - return fmt.Errorf("vault not found") - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
cmd/dca/server/main.go(1 hunks)cmd/fees/server/main.go(1 hunks)cmd/fees/worker/config.go(1 hunks)cmd/fees/worker/main.go(2 hunks)docker-compose.yaml(1 hunks)init-scripts/02_create_vultisig_verifier.sql(1 hunks)plugin/common/common.go(1 hunks)plugin/common/constants.go(1 hunks)plugin/common/nonce.go(1 hunks)plugin/fees/fees.go(8 hunks)plugin/fees/transaction.go(1 hunks)scripts/dev/create_fee_policy/dummy_server.go(1 hunks)storage/postgres/migrations/plugin/20250630152230_fee_runs.sql(1 hunks)storage/postgres/schema/schema.sql(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- init-scripts/02_create_vultisig_verifier.sql
- cmd/fees/worker/config.go
- scripts/dev/create_fee_policy/dummy_server.go
- docker-compose.yaml
- cmd/dca/server/main.go
- plugin/common/constants.go
- cmd/fees/server/main.go
- storage/postgres/migrations/plugin/20250630152230_fee_runs.sql
- cmd/fees/worker/main.go
- plugin/common/nonce.go
- plugin/common/common.go
- storage/postgres/schema/schema.sql
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
plugin/fees/transaction.go (5)
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.
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.
Learnt from: webpiratt
PR: vultisig/plugin#105
File: plugin/dca/dca.go:734-736
Timestamp: 2025-07-01T17:35:35.277Z
Learning: The DCA plugin in the vultisig/plugin codebase is currently not fully implemented. Methods like getCompletedSwapTransactionsCount() return placeholder values (e.g., 0) with TODO comments, which is expected behavior during the development phase. The plugin implementation is intentionally incomplete while other parts of the system are being refactored.
Learnt from: webpiratt
PR: 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.
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.
plugin/fees/fees.go (6)
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.
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.
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.
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.
Learnt from: johnnyluo
PR: vultisig/plugin#108
File: Dockerfile.Payroll.server:14-19
Timestamp: 2025-07-02T04:55:36.331Z
Learning: In the vultisig/plugin repository, the team maintains both the main repository and the go-wrappers dependency repository, so they are comfortable downloading from the master branch rather than pinning to specific commits.
Learnt from: 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.
🧬 Code Graph Analysis (1)
plugin/fees/transaction.go (6)
plugin/fees/fees.go (1)
FeePlugin(52-65)internal/types/keysign.go (1)
PluginKeysignRequest(7-12)common/vault.go (1)
GetVaultFromPolicy(12-24)plugin/common/common.go (1)
GenUnsignedTx(27-53)internal/scheduler/scheduler.go (1)
NewIntervalSchedule(271-286)internal/tasks/tasks.go (2)
TypeKeySignDKLS(13-13)QUEUE_NAME(9-9)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (7)
plugin/fees/transaction.go (2)
162-169: Verify transaction hash generation.Using
txHexdirectly as the hash may not produce a valid transaction hash. Transaction hashes are typically computed by hashing the RLP-encoded transaction data.Consider whether this should use a proper hash calculation or if the current approach is intentional for this use case.
226-290: Well-implemented asynchronous signing workflow.The method properly handles task marshaling, queueing, polling, and result processing with appropriate error handling and timeouts.
plugin/fees/fees.go (5)
68-104: Constructor properly handles expanded dependencies.The updated constructor correctly validates new dependencies and initializes the nonce manager and other required components for the enhanced fee collection workflow.
106-145: Comprehensive recipe specification.The method properly defines the plugin's capabilities, supported resources, and parameter constraints in a well-structured format.
209-242: Solid fee validation and collection logic.The method properly fetches fees from the verifier API, validates the sum consistency, and handles the case where no fees are pending collection.
296-334: Well-implemented comprehensive fee collection.The method correctly iterates through all public keys, validates fee amounts, and delegates to
executeFeeCollectionfor keys with pending fees.
386-397: Excellent concurrent signing implementation.The use of
errgroupto handle concurrent signing requests is well-implemented with proper error handling and theasynq.SkipRetryreturn pattern.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
plugin/fees/transaction.go (1)
122-125: Fix inconsistent receiver reference in error message.Line 122 correctly uses
fpbut the error message on line 124 still referencesp.- return []vtypes.PluginKeysignRequest{}, fmt.Errorf("p.nonceManager.GetNextNonce: %w", err) + return []vtypes.PluginKeysignRequest{}, fmt.Errorf("fp.nonceManager.GetNextNonce: %w", err)
🧹 Nitpick comments (3)
plugin/fees/transaction.go (1)
142-142: Consider adding "0x" prefix for hex-encoded transaction data.Based on the learning that signRequest.Transaction should be stored with "0x" prefix for compatibility with gcommon.FromHex, consider prefixing the hex-encoded transaction.
-txHex := hex.EncodeToString(tx) +txHex := "0x" + hex.EncodeToString(tx)Also applies to: 164-164, 176-176
plugin/fees/fees.go (2)
68-68: Consider reducing constructor complexity.The constructor has 9 parameters, which makes it difficult to use and maintain. Consider using a configuration struct or builder pattern.
type FeePluginConfig struct { DB storage.DatabaseStorage Logger logrus.FieldLogger BaseConfigPath string VaultStorage *vault.BlockStorageImp TxIndexerService *tx_indexer.Service Inspector *asynq.Inspector Client *asynq.Client FeeConfig *FeeConfig EncryptionSecret string } func NewFeePlugin(config FeePluginConfig) (*FeePlugin, error) { // validation and initialization }
278-280: Complete or remove commented transaction building code.The commented code suggests incomplete implementation of transaction building. Either implement this functionality or remove the dead code.
-// fp.buildUSDCEthFeeTransaction(feesToCollect) - -// fp.Log(logrus.InfoLevel, "Fees to collect: ", feesToCollect)Would you like me to help implement the transaction building logic or should this code be removed?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
cmd/dca/server/main.go(1 hunks)cmd/fees/server/main.go(1 hunks)cmd/fees/worker/config.go(1 hunks)cmd/fees/worker/main.go(2 hunks)docker-compose.yaml(1 hunks)init-scripts/02_create_vultisig_verifier.sql(1 hunks)plugin/common/common.go(1 hunks)plugin/common/constants.go(1 hunks)plugin/common/nonce.go(1 hunks)plugin/fees/fees.go(8 hunks)plugin/fees/transaction.go(1 hunks)scripts/dev/create_fee_policy/dummy_server.go(1 hunks)storage/postgres/migrations/plugin/20250630152230_fee_runs.sql(1 hunks)storage/postgres/schema/schema.sql(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- init-scripts/02_create_vultisig_verifier.sql
- cmd/fees/worker/config.go
- scripts/dev/create_fee_policy/dummy_server.go
- cmd/dca/server/main.go
- docker-compose.yaml
- cmd/fees/server/main.go
- plugin/common/constants.go
- plugin/common/nonce.go
- storage/postgres/migrations/plugin/20250630152230_fee_runs.sql
- plugin/common/common.go
- storage/postgres/schema/schema.sql
- cmd/fees/worker/main.go
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
plugin/fees/transaction.go (5)
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.
Learnt from: webpiratt
PR: vultisig/plugin#105
File: plugin/dca/dca.go:734-736
Timestamp: 2025-07-01T17:35:35.277Z
Learning: The DCA plugin in the vultisig/plugin codebase is currently not fully implemented. Methods like getCompletedSwapTransactionsCount() return placeholder values (e.g., 0) with TODO comments, which is expected behavior during the development phase. The plugin implementation is intentionally incomplete while other parts of the system are being refactored.
Learnt from: webpiratt
PR: 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.
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.
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.
plugin/fees/fees.go (6)
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.
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.
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.
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.
Learnt from: johnnyluo
PR: vultisig/plugin#108
File: Dockerfile.Payroll.server:14-19
Timestamp: 2025-07-02T04:55:36.331Z
Learning: In the vultisig/plugin repository, the team maintains both the main repository and the go-wrappers dependency repository, so they are comfortable downloading from the master branch rather than pinning to specific commits.
Learnt from: 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.
🔇 Additional comments (1)
plugin/fees/fees.go (1)
341-341: Verify the onlyActive parameter usage.The method correctly uses
truefor theonlyActiveparameter when fetching plugin policies, which ensures only active policies are processed for fee collection.
| if e != nil { | ||
| return []vtypes.PluginKeysignRequest{}, fmt.Errorf("p.genUnsignedTx: %w", e) | ||
| } |
There was a problem hiding this comment.
Fix incorrect function reference in error message.
The error message references p.genUnsignedTx but the actual function called is plugincommon.GenUnsignedTx.
- return []vtypes.PluginKeysignRequest{}, fmt.Errorf("p.genUnsignedTx: %w", e)
+ return []vtypes.PluginKeysignRequest{}, fmt.Errorf("plugincommon.GenUnsignedTx: %w", e)📝 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 e != nil { | |
| return []vtypes.PluginKeysignRequest{}, fmt.Errorf("p.genUnsignedTx: %w", e) | |
| } | |
| if e != nil { | |
| return []vtypes.PluginKeysignRequest{}, fmt.Errorf("plugincommon.GenUnsignedTx: %w", e) | |
| } |
🤖 Prompt for AI Agents
In plugin/fees/transaction.go around lines 138 to 140, the error message
incorrectly references the function as p.genUnsignedTx while the actual function
called is plugincommon.GenUnsignedTx. Update the error message string to
correctly reflect plugincommon.GenUnsignedTx instead of p.genUnsignedTx to
accurately indicate the source of the error.
| for { | ||
| select { | ||
| case <-ctx.Done(): | ||
| return ctx.Err() | ||
| case <-time.After(3 * time.Second): | ||
| taskInfo, er := fp.asynqInspector.GetTaskInfo(tasks.QUEUE_NAME, task.ID) | ||
| if er != nil { | ||
| fp.logger.WithError(er).Error("p.inspector.GetTaskInfo(tasks.QUEUE_NAME, task.ID)") | ||
| return fmt.Errorf("p.inspector.GetTaskInfo: %w", er) | ||
| } | ||
| if taskInfo.State != asynq.TaskStateCompleted { | ||
| continue | ||
| } | ||
| if taskInfo.Result == nil { | ||
| fp.logger.Info("taskInfo.Result is nil, skipping") | ||
| return nil | ||
| } | ||
|
|
||
| var res map[string]tss.KeysignResponse | ||
| er = json.Unmarshal(taskInfo.Result, &res) | ||
| if er != nil { | ||
| fp.logger.WithError(er).Error("json.Unmarshal(taskInfo.Result, &res)") | ||
| return fmt.Errorf("json.Unmarshal(taskInfo.Result, &res): %w", er) | ||
| } | ||
|
|
||
| var sig tss.KeysignResponse | ||
| for _, v := range res { // one sig for evm (map with 1 key) | ||
| sig = v | ||
| } | ||
|
|
||
| er = fp.SigningComplete(ctx, sig, req, pluginPolicy) | ||
| if er != nil { | ||
| fp.logger.WithError(er).Error("p.SigningComplete") | ||
| return fmt.Errorf("p.SigningComplete: %w", er) | ||
| } | ||
|
|
||
| fp.logger.WithField("public_key", req.PublicKey). | ||
| Info("successfully signed and broadcasted") | ||
| return nil | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve polling mechanism robustness.
The current polling implementation uses a simple 3-second interval. Consider adding exponential backoff and a maximum polling duration to prevent infinite loops.
-case <-time.After(3 * time.Second):
+case <-time.After(time.Duration(min(3*attempts, 30)) * time.Second):Also consider setting a maximum polling duration:
+maxPollDuration := 5 * time.Minute
+pollStart := time.Now()
for {
select {
case <-ctx.Done():
return ctx.Err()
+ case <-time.After(maxPollDuration):
+ if time.Since(pollStart) > maxPollDuration {
+ return fmt.Errorf("polling timeout after %v", maxPollDuration)
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In plugin/fees/transaction.go between lines 249 and 289, the polling loop uses a
fixed 3-second interval without a timeout, risking infinite loops. Modify the
code to implement exponential backoff by increasing the wait time after each
unsuccessful poll, and add a maximum polling duration or deadline after which
the loop exits with an error or timeout. Use context deadlines or timers to
enforce the maximum duration and adjust the time.After interval dynamically for
backoff.
| if e != nil { | ||
| return []vtypes.PluginKeysignRequest{}, fmt.Errorf("p.txIndexerService.CreateTx: %w", e) | ||
| } |
There was a problem hiding this comment.
Fix inconsistent receiver reference in error message.
The error message should use fp consistently with the receiver name.
- return []vtypes.PluginKeysignRequest{}, fmt.Errorf("p.txIndexerService.CreateTx: %w", e)
+ return []vtypes.PluginKeysignRequest{}, fmt.Errorf("fp.txIndexerService.CreateTx: %w", e)📝 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 e != nil { | |
| return []vtypes.PluginKeysignRequest{}, fmt.Errorf("p.txIndexerService.CreateTx: %w", e) | |
| } | |
| if e != nil { | |
| return []vtypes.PluginKeysignRequest{}, fmt.Errorf("fp.txIndexerService.CreateTx: %w", e) | |
| } |
🤖 Prompt for AI Agents
In plugin/fees/transaction.go around lines 153 to 155, the error message uses an
inconsistent receiver name 'p' instead of 'fp'. Update the error message to use
'fp.txIndexerService.CreateTx' to match the receiver name and maintain
consistency.
| chain, | ||
| policy.PluginID, | ||
| policy.ID, | ||
| token.Address, //TODO check if this is symbol or address |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Clarify token identification in transaction indexer.
The TODO comment indicates uncertainty about whether to use token symbol or address. Based on the context where token.Address is used consistently throughout the method, this should be the contract address.
🏁 Script executed:
#!/bin/bash
# Verify how tokenID is used in the transaction indexer service
ast-grep --pattern 'GetTxInTimeRange($$$, tokenID, $$$)'Length of output: 58
Update TODO to clarify contract address usage
The token.Address field is consistently used as the tokenID (i.e. contract address) when calling the indexer:
• Service signature (plugin/fees/service/tx_indexer.go):
func (s *Service) GetTxInTimeRange(ctx context.Context, tokenID string, start, end time.Time) ([]Tx, error)
• Invocation (plugin/fees/transaction.go):
- token.Address, //TODO check if this is symbol or address
+ token.Address, // contract addressReplace the TODO with an explicit note that this is the contract address.
📝 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.
| token.Address, //TODO check if this is symbol or address | |
| token.Address, // contract address |
🤖 Prompt for AI Agents
In plugin/fees/transaction.go at line 107, replace the TODO comment next to
token.Address with a clear note stating that token.Address is used as the
contract address (tokenID) when calling the indexer. This clarifies the purpose
of token.Address and removes ambiguity about whether it is a symbol or address.
| keySignRequests, err := fp.ProposeTransactions(feePolicy) | ||
| if err != nil { | ||
| fp.Log(logrus.DebugLevel, "Failed to propose transactions: ", err) | ||
| } | ||
| fp.Log(logrus.DebugLevel, "keySignRequest: ", keySignRequests) |
There was a problem hiding this comment.
Handle ProposeTransactions failure more gracefully.
The method logs the error but continues execution even when ProposeTransactions fails. This could lead to inconsistent state.
keySignRequests, err := fp.ProposeTransactions(feePolicy)
if err != nil {
fp.Log(logrus.DebugLevel, "Failed to propose transactions: ", err)
+ return fmt.Errorf("failed to propose transactions: %w", err)
}📝 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.
| keySignRequests, err := fp.ProposeTransactions(feePolicy) | |
| if err != nil { | |
| fp.Log(logrus.DebugLevel, "Failed to propose transactions: ", err) | |
| } | |
| fp.Log(logrus.DebugLevel, "keySignRequest: ", keySignRequests) | |
| keySignRequests, err := fp.ProposeTransactions(feePolicy) | |
| if err != nil { | |
| fp.Log(logrus.DebugLevel, "Failed to propose transactions: ", err) | |
| return fmt.Errorf("failed to propose transactions: %w", err) | |
| } | |
| fp.Log(logrus.DebugLevel, "keySignRequest: ", keySignRequests) |
🤖 Prompt for AI Agents
In plugin/fees/fees.go around lines 380 to 384, the code logs an error if
ProposeTransactions fails but continues execution, which may cause inconsistent
state. Modify the code to handle the error more gracefully by returning the
error or stopping further processing when ProposeTransactions returns an error,
ensuring the function does not proceed with invalid or incomplete data.
| feeRun, err := fp.db.CreateFeeRun(ctx, feePolicy.ID, types.FeeRunStateDraft, feeIds) | ||
| if err != nil { | ||
| fp.Log(logrus.DebugLevel, "Failed to create fee run: ", err) | ||
| return fmt.Errorf("failed to create fee run: %w", err) | ||
| } | ||
| fp.Log(logrus.DebugLevel, "Fee run created: ", feeRun) | ||
|
|
||
| //Get vault and check it exists | ||
|
|
||
| vaultFileName := vcommon.GetVaultBackupFilename(ecdsaPublicKey, vtypes.PluginVultisigFees_feee.String()) | ||
| vaultContent, err := fp.vaultStorage.GetVault(vaultFileName) | ||
| if err != nil { | ||
| // TODO some real error handling here | ||
| fp.Log(logrus.DebugLevel, "Failed to get vault: ", err) | ||
| return fmt.Errorf("failed to get vault: %w", err) | ||
| } | ||
| if vaultContent == nil { | ||
| // TODO some real error handling here | ||
| fp.Log(logrus.DebugLevel, "Vault not found") | ||
| return fmt.Errorf("vault not found") | ||
| } | ||
|
|
||
| keySignRequests, err := fp.ProposeTransactions(feePolicy) | ||
| if err != nil { | ||
| fp.Log(logrus.DebugLevel, "Failed to propose transactions: ", err) | ||
| } | ||
| fp.Log(logrus.DebugLevel, "keySignRequest: ", keySignRequests) | ||
|
|
||
| var eg errgroup.Group | ||
| for _, keySignRequest := range keySignRequests { | ||
| req := keySignRequest | ||
| eg.Go(func() error { | ||
| return fp.initSign(ctx, req, feePolicy) | ||
| }) | ||
| } | ||
| err = eg.Wait() | ||
| if err != nil { | ||
| fp.logger.WithError(err).Error("eg.Wait") | ||
| return fmt.Errorf("eg.Wait: %s, %w", err, asynq.SkipRetry) | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and resource cleanup in fee collection.
The method creates a fee run but doesn't handle cleanup if subsequent operations fail. If ProposeTransactions or signing fails, the fee run remains in draft state.
Consider adding cleanup logic:
feeRun, err := fp.db.CreateFeeRun(ctx, feePolicy.ID, types.FeeRunStateDraft, feeIds)
if err != nil {
fp.Log(logrus.DebugLevel, "Failed to create fee run: ", err)
return fmt.Errorf("failed to create fee run: %w", err)
}
+
+// Ensure cleanup on failure
+defer func() {
+ if err != nil {
+ // Update fee run status to failed or delete it
+ fp.logger.WithError(err).Error("Fee collection failed, cleaning up fee run")
+ }
+}()Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In plugin/fees/fees.go around lines 358 to 398, the code creates a fee run but
lacks cleanup if subsequent steps like ProposeTransactions or signing fail,
leaving the fee run in draft state. To fix this, add error handling that deletes
or rolls back the created fee run when these later operations fail. Implement
cleanup logic after each failure point following fee run creation to ensure no
stale draft fee runs remain.
| Fees []FeeDto `json:"fees" validate:"required"` | ||
| TotalFeesIncurred int `json:"total_fees_incurred" validate:"required"` // Total fees incurred in the smallest unit, e.g., "1000000" for 0.01 VULTI | ||
| FeesPendingCollection int `json:"fees_pending_collection" validate:"required"` // Total fees pending collection in the smallest unit, e.g., "1000000" for 0.01 VULTI | ||
| // PolicyId uuid.UUID `json:"policy_id" validate:"required"` |
There was a problem hiding this comment.
do you want to keep this?
| return feeHistory.Data, nil | ||
| } | ||
|
|
||
| // TODO add auth |
| v.logger.Debug("URL: ", url) | ||
| response, err := v.get(url) | ||
| if err != nil { | ||
| v.logger.WithError(err).Error("failed to get public key fees") |
There was a problem hiding this comment.
If you already return a wrapped error , don't log it here , whoever call GetPublicKeysFees to log it
otherwise you double log it
| fp.logger.Debug("Fees response: ", feesResponse) | ||
|
|
||
| for publicKey, feeHistory := range feesResponse { | ||
| //TODO just for testing |
There was a problem hiding this comment.
is it relevant? if it is just for testing , then we should not commit it , otherwise remove the comments
| } | ||
| err = eg.Wait() | ||
| if err != nil { | ||
| fp.logger.WithError(err).Error("eg.Wait") |
There was a problem hiding this comment.
in the same file , sometimes you use fp.logger, sometims you use fp.Log
Consistency....
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Documentation
Refactor