Ongoing fee work#98
Conversation
WalkthroughThis update introduces a comprehensive fee collection plugin system for a vault-based application. It adds new configuration modules, a fee plugin with asynchronous task handling, verifier API integration, and supporting scripts for policy creation and testing. The changes also update configuration files, Docker, and SQL scripts, and centralize utility functions for policy encoding and validation. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FeeWorker
participant Redis
participant FeePlugin
participant VerifierAPI
participant Vault
participant Database
User->>FeeWorker: Enqueue fee collection task (via Asynq)
FeeWorker->>Redis: Fetch fee collection task
FeeWorker->>FeePlugin: HandleCollections(task)
FeePlugin->>VerifierAPI: GetPluginPolicyFees(policyId)
VerifierAPI-->>FeePlugin: Fee history (pending, collected)
FeePlugin->>Vault: (if needed) Access vault for signing
FeePlugin->>Database: (if needed) Update fee collection status
FeePlugin-->>FeeWorker: Task result (success/failure)
Possibly related PRs
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)level=warning msg="[runner] Can't run linter goanalysis_metalinter: buildir: failed to load package session: could not load export data: no export data for "github.com/vultisig/go-wrappers/go-dkls/sessions"" 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
✨ 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 (
|
There was a problem hiding this comment.
Pull Request Overview
This PR establishes the groundwork for a new fee‐collection plugin by adding scripts, server stubs, plugin skeletons, configuration options, and deployment updates.
- Introduces a
create_fee_policyscript and a dummy server for local testing - Adds a
feesplugin skeleton with configuration, task handling, and API integrations - Updates deployment artifacts (SQL scripts, Docker, buckets, compose, examples)
Reviewed Changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/dev/create_fee_policy/main.go | CLI to generate and optionally store a fee policy |
| scripts/dev/create_fee_policy/dummy_server.go | Creates a dummy server environment for local tests |
| plugin/fees/transaction.go | Partial buildUSDCEthFeeTransaction implementation |
| plugin/fees/fees.go | Fee plugin core methods and task handler stubs |
| plugin/fees/constraints.go | Fee collection constants and ABI placeholder |
| plugin/fees/config.go | FeeConfig creation helpers and defaults |
| internal/verifierapi/verifierapi.go | Base HTTP client for verifier API calls |
| internal/verifierapi/fees.go | Fetches fee history from verifier service |
| init-scripts/02_create_vultisig_verifier.sql | Add IF NOT EXISTS to DB creation |
| init-scripts/01_create_vultisig_plugin.sql | Add IF NOT EXISTS to DB creation |
| fee_config.example.json | Example JSON for fee plugin configuration |
| docker-compose.yaml | Set POSTGRES_DB to vultisig-plugin |
| create_buckets.sh | mc mb with --ignore-existing |
| config.example.json | Core server config example |
| config-plugin.yaml | Plugin mode and vault path adjustments |
| common/util.go | Shared helpers for policy encoding |
| cmd/fees/worker/main.go | Fee worker Asynq setup and demo enqueue |
| cmd/fees/worker/config.go | Worker config loader |
| cmd/fees/server/main.go | Fee server bootstrap |
| cmd/fees/server/config.go | Server config loader |
| api/server_config.go | Add VerifierUrl and VaultsFilePath to config |
| api/plugin.go | Use common.PolicyToMessageHex for signature check |
| Dockerfile.postgres | Pin to Postgres 17 |
Comments suppressed due to low confidence (3)
scripts/dev/create_fee_policy/main.go:136
- The variable name
jsonshadows the importedencoding/jsonpackage. Consider renaming the variable (e.g.,policyJSONorjsonBytes).
json, err := json.MarshalIndent(pluginPolicy, "", " ")
plugin/fees/config.go:94
- The code calls
errors.Newbut theerrorspackage is not imported. Addimport "errors".
}
plugin/fees/config.go:124
- The code uses
strings.NewReplacerbut thestringspackage is not imported. Addimport "strings".
v.SetEnvKeyReplacer(strings.NewReplacer(".", "_"))
| defer func() { | ||
| if err := client.Close(); err != nil { | ||
| fmt.Println("fail to close asynq client,", err) | ||
| } | ||
| }() |
There was a problem hiding this comment.
Deferring client.Close() inside this constructor will close the Asynq client immediately when createDummyServer returns, likely causing the server to operate on a closed client. Remove the defer and handle closing at the appropriate program shutdown point.
| defer func() { | |
| if err := client.Close(); err != nil { | |
| fmt.Println("fail to close asynq client,", err) | |
| } | |
| }() |
| } | ||
|
|
||
| //TODO garry | ||
| data, err := erc20ABI.Pack("transfer", fp.config.CollectorAddress) |
There was a problem hiding this comment.
The ERC-20 transfer ABI requires both a recipient address and an amount to transfer. You are only packing the recipient; you must include the amount parameter.
| data, err := erc20ABI.Pack("transfer", fp.config.CollectorAddress) | |
| transferAmount := big.NewInt(int64(amount)) // Convert amount to *big.Int | |
| data, err := erc20ABI.Pack("transfer", fp.config.CollectorAddress, transferAmount) |
| } | ||
|
|
||
| ethFromAddress := ecommon.HexToAddress(ethFromAddressString) | ||
| nonce, err := fp.rpcClient.PendingNonceAt(context.Background(), ethFromAddress) |
There was a problem hiding this comment.
The error returned by PendingNonceAt is not checked before reassigning err on the next call. Insert an if err != nil check immediately after the nonce retrieval.
| nonce, err := fp.rpcClient.PendingNonceAt(context.Background(), ethFromAddress) | |
| nonce, err := fp.rpcClient.PendingNonceAt(context.Background(), ethFromAddress) | |
| if err != nil { | |
| return fmt.Errorf("failed to retrieve nonce: %w", err) | |
| } |
RaghavSood
left a comment
RaghavSood
left a comment
There was a problem hiding this comment.
Broadly LGTM, one nit, non-blocking
| Port int64 `mapstructure:"port" json:"port,omitempty"` | ||
| EncryptionSecret string `mapstructure:"encryption_secret" json:"encryption_secret,omitempty"` | ||
| VerifierUrl string `mapstructure:"verifier_url" json:"verifier_url,omitempty"` //The url of the verifier (i.e. the counter party to sign transactions). | ||
| VaultsFilePath string `mapstructure:"vaults_file_path" json:"vaults_file_path,omitempty"` //This is just for testing locally |
There was a problem hiding this comment.
Can we create an issue to remove this once s3 testing buckets are available?
| }, | ||
| ) | ||
|
|
||
| postgressDB, err := postgres.NewPostgresBackend(cfg.Database.DSN, nil) |
There was a problem hiding this comment.
| postgressDB, err := postgres.NewPostgresBackend(cfg.Database.DSN, nil) | |
| postgresDB, err := postgres.NewPostgresBackend(cfg.Database.DSN, nil) |
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation