🚚 Latest type imports, fixed names and policy version#93
Conversation
WalkthroughThe changes standardize the use of the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Plugin (DCA/Payroll)
Note over Plugin: All methods now accept PluginPolicy (int PolicyVersion)
Caller->>Plugin: ProposeTransactions(policy: PluginPolicy)
Plugin-->>Caller: []PluginKeysignRequest, error
Caller->>Plugin: ValidatePluginPolicy(policyDoc: PluginPolicy)
Plugin-->>Caller: error
Caller->>Plugin: ValidateProposedTransactions(policy: PluginPolicy, txs)
Plugin-->>Caller: error
Caller->>Plugin: SigningComplete(ctx, signature, signRequest, policy: PluginPolicy)
Plugin-->>Caller: error
Possibly related PRs
Suggested reviewers
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"" ✨ 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 updates type imports, fixes naming, and adjusts policy version handling to address breaking changes introduced in verifier.
- Changes PolicyVersion fields from string to int in tests and policy creation scripts
- Updates function signatures to use PluginPolicy instead of PluginPolicyCreateUpdate in payroll and DCA plugins
- Updates dependency versions in go.mod to align with the new verifier changes
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| storage/postgres/time_trigger_test.go | Updated PolicyVersion type from string to int for test data |
| scripts/dev/create_payroll_policy/main.go | Updated PolicyVersion type from string to int in payroll policy creation |
| scripts/dev/create_dca_policy/main.go | Updated PolicyVersion type from string to int in DCA policy creation |
| plugin/payroll/transaction.go | Changed function signatures to use PluginPolicy, removing ToPluginPolicy calls |
| plugin/payroll/policy.go | Updated validation functions to use PluginPolicy |
| plugin/dca/dca.go | Updated function signatures and adjusted PolicyVersion handling logic |
| go.mod | Upgraded dependency versions to support the breaking changes |
| if fmt.Sprintf("%d", policyDoc.PolicyVersion) != policyVersion { //TODO policy version will essentially be a nonce that is incremented by 1, not, like plugin versions that will follow traditional engineering versioning e.g. v2.12.3. For now we can compare string types | ||
| return fmt.Errorf("policy does not match policy version, expected: %s, got: %s", policyVersion, policyDoc.PolicyVersion) |
There was a problem hiding this comment.
[nitpick] Consider revising the conversion and comparison logic for PolicyVersion. If possible, compare integers directly or refactor the code to use a consistent type, rather than converting the integer to a string.
| if fmt.Sprintf("%d", policyDoc.PolicyVersion) != policyVersion { //TODO policy version will essentially be a nonce that is incremented by 1, not, like plugin versions that will follow traditional engineering versioning e.g. v2.12.3. For now we can compare string types | |
| return fmt.Errorf("policy does not match policy version, expected: %s, got: %s", policyVersion, policyDoc.PolicyVersion) | |
| if policyDoc.PolicyVersion != policyVersion { // Policy version is now compared as an integer | |
| return fmt.Errorf("policy does not match policy version, expected: %d, got: %d", policyVersion, policyDoc.PolicyVersion) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (7)
scripts/dev/create_dca_policy/main.go (1)
72-82:⚠️ Potential issueMissing
Recipeassignment will break policy validation
policy.Recipeis still the empty string. Both the verifier and the plugin decode & validate this field – requests will immediately fail with “failed to decode policy recipe”.@@ policy := vtypes.PluginPolicy{ ID: policyId, PublicKey: key, PluginID: vtypes.PluginVultisigDCA_0000, PluginVersion: "1.0.0", - PolicyVersion: 1, + PolicyVersion: 1, Active: true, Signature: "0x0000000000000000000000000000000000000000000000000000000000000000", } @@ fmt.Println("DCA policy", string(policyBytes)) + + // Attach recipe to policy (base64-url w/o padding, which verifier expects) + policy.Recipe = base64.RawStdEncoding.EncodeToString(policyBytes) + + // missing import + // import "encoding/base64"scripts/dev/create_payroll_policy/main.go (1)
96-105:⚠️ Potential issue
Recipenot populated – policy creation will failSame issue as in the DCA helper:
policy.Recipeis never set. The verifier rejects policies without a decodable recipe.@@ policy := vtypes.PluginPolicy{ ID: policyId, PublicKey: key, PluginID: vtypes.PluginVultisigPayroll_0000, PluginVersion: "1.0.0", - PolicyVersion: 1, + PolicyVersion: 1, Active: true, Signature: "0x0000000000000000000000000000000000000000000000000000000000000000", } @@ fmt.Println("Payroll policy", string(policyBytes)) - // policy.Policy = policyBytes + + // Attach recipe in the format expected by verifier + policy.Recipe = base64.RawStdEncoding.EncodeToString(policyBytes) + + // add import + // import "encoding/base64"plugin/payroll/policy.go (1)
40-105:⚠️ Potential issue
payrollPolicynever initialised – will panic on first Tx
payrollPolicyis declared but never populated; any access topayrollPolicy.TokenID[i]orRecipientscauses an index-out-of-range or nil-value error as soon aslen(txs) > 0.Proposed quick fix: parse the recipe into the strongly-typed struct before validating transactions.
- var payrollPolicy PayrollPolicy - // TODO: convert the recipes to PayrollPolicy + var payrollPolicy PayrollPolicy + // Decode base64-encoded JSON recipe + policyBytes, err := base64.RawStdEncoding.DecodeString(policy.Recipe) + if err != nil { + return fmt.Errorf("failed to decode policy recipe: %w", err) + } + if err := json.Unmarshal(policyBytes, &payrollPolicy); err != nil { + return fmt.Errorf("failed to unmarshal payroll policy: %w", err) + } + + // ensure the recipe length matches the number of proposed txs + if len(payrollPolicy.TokenID) != len(txs) { + return fmt.Errorf("recipe contains %d token IDs but %d transactions supplied", len(payrollPolicy.TokenID), len(txs)) + }Add missing import:
import ( "encoding/base64" "encoding/json" // existing imports… )Without this fix the method will compile but always fail at runtime.
plugin/payroll/transaction.go (3)
61-80:⚠️ Potential issue
payrollPolicyis never initialised – all subsequent field access is undefined
var payrollPolicy PayrollPolicyleaves every slice (Recipients,ChainID,TokenID) nil / empty.
Theforloop therefore produces zero transactions, but the function later blindly indexestxs[0], causing a panic.Either:
- Populate
payrollPolicyfrom the recipe before use, or- Return an explicit error when the conversion is still TODO.
- var payrollPolicy PayrollPolicy - // TODO: convert the recipes to PayrollPolicy + payrollPolicy, err := convertRecipeToPayrollPolicy(policy) // helper to be implemented + if err != nil { + return nil, fmt.Errorf("failed to build payroll policy: %w", err) + }
103-110:⚠️ Potential issuePotential panic: dereferencing
txs[0]without verifying lengthIf no transactions were appended (see previous comment),
txs[0]triggers anindex out of rangepanic.-if len(txs) == 0 { - return nil, fmt.Errorf("no transactions generated") -} -signRequest := txs[0] +if len(txs) == 0 { + return nil, fmt.Errorf("no transactions generated") +} +signRequest := txs[0]
305-309:⚠️ Potential issueUninitialised
payrollPolicy.ChainID[0]leads to slice-bounds panic
payrollPolicyis still empty insideconvertData; indexing[0]will panic.
Retrieve the chain-ID directly fromoriginalTx(already available) or ensure the policy is properly converted before calling this helper.- chainID = new(big.Int) - chainID.SetString(payrollPolicy.ChainID[0], 10) +chainID = originalTx.ChainId()plugin/dca/dca.go (1)
143-149:⚠️ Potential issueVersion checks compare incompatible types – will always fail
pluginVersion/policyVersionare string constants"0.0.1", whereaspolicyDoc.PluginVersionand.PolicyVersionhave been migrated to numeric fields (int32/int).
Current comparisons will never pass, causing every policy to be rejected.- if policyDoc.PluginVersion != pluginVersion { + if fmt.Sprintf("%d", policyDoc.PluginVersion) != pluginVersion { // temporary workaround…still mismatches. Prefer comparing integers and remove the semantic-version strings altogether:
-const ( - pluginVersion = "0.0.1" - policyVersion = "0.0.1" -) +const ( + pluginVersion = 1 // numeric + policyVersion = 1 +) ... - if policyDoc.PluginVersion != pluginVersion { + if policyDoc.PluginVersion != pluginVersion { ... - if fmt.Sprintf("%d", policyDoc.PolicyVersion) != policyVersion { + if policyDoc.PolicyVersion != policyVersion {
🧹 Nitpick comments (2)
plugin/payroll/transaction.go (1)
55-61: *Pass vtypes.PluginPolicy by pointer to avoid an unnecessary copy
PluginPolicyis not a tiny struct (it embeds recipes, schedules, etc.).
Passing it by value here copies the entire struct every call, whereas every use inside the method is read-only. Consider switching to a pointer (as was done for other heavy params such ascontext.Context).-func (p *PayrollPlugin) ProposeTransactions(policy vtypes.PluginPolicy) ([]vtypes.PluginKeysignRequest, error) { +func (p *PayrollPlugin) ProposeTransactions(policy *vtypes.PluginPolicy) ([]vtypes.PluginKeysignRequest, error) {plugin/dca/dca.go (1)
273-309:completePolicymutates a copy — changes are lost
policyis passed by value toProposeTransactions;completePolicyreceives that copy.
InsidecompletePolicythe struct’sActivefield is flipped tofalse, but once the function returns the outer copy remains unchanged.Either:
- Accept
*vtypes.PluginPolicyeverywhere, or- Return the updated struct and persist it in the caller.
Tagging as lower priority since the DB update does persist the change, but the in-memory copy may later be reused with stale state.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
go.mod(1 hunks)plugin/dca/dca.go(7 hunks)plugin/payroll/policy.go(2 hunks)plugin/payroll/transaction.go(2 hunks)scripts/dev/create_dca_policy/main.go(1 hunks)scripts/dev/create_payroll_policy/main.go(1 hunks)storage/postgres/time_trigger_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
plugin/payroll/policy.go (2)
plugin/payroll/payroll.go (1)
PayrollPlugin(15-21)internal/types/keysign.go (1)
PluginKeysignRequest(7-12)
plugin/payroll/transaction.go (2)
plugin/payroll/payroll.go (1)
PayrollPlugin(15-21)internal/types/keysign.go (1)
PluginKeysignRequest(7-12)
plugin/dca/dca.go (1)
internal/types/keysign.go (1)
PluginKeysignRequest(7-12)
🔇 Additional comments (3)
storage/postgres/time_trigger_test.go (1)
41-47:PolicyVersionmigrated toint– looks correct
InsertPluginPolicyTxnow receivesPolicyVersion: 1(int) which matches the new verifier API. No further issues spotted in this hunk.go.mod (1)
22-23: Dependency bumps – rungo mod tidy& CI before mergeTwo internal modules were upgraded. Please:
go mod tidyto ensure no stale requirements.- Re-run unit tests &
go vet– both upstream repos often introduce breaking API tweaks between commits.plugin/payroll/policy.go (1)
199-230: Interface alignment checkThe signature change to
ValidatePluginPolicy(policyDoc vtypes.PluginPolicy)matches the newverifierAPI. Confirm thatPayrollPluginstill satisfies whatever interface the host expects (e.g.plugin.Plugin). A quickgo vetrun will flag any mismatch.
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
api/plugin.go (1)
195-217:⚠️ Potential issueMissing replay-protection: policy signature is verified after potential mutation
policy.IDmay be autogenerated (lines 207-209) before signature verification (line 211).
If clients sign a payload withoutID, the server mutates the struct and then validates the signature against mutated data → always fails.Either:
- Require
IDto be present and signed, or- Generate
IDbefore clients sign, or- Exclude
IDfrom the signed fields.
♻️ Duplicate comments (1)
api/plugin.go (1)
239-245: Same replay-protection concern for Update pathSee comment above – signature is verified after possible field mutations, leading to systematic verification failures.
🧹 Nitpick comments (4)
plugin/fees/fees.go (2)
41-44: Logger field is copy-pasted from Payroll plugin
logrus.WithField("plugin", "payroll")breaks filtering/metrics for the new “fees” plugin.-logger: logrus.WithField("plugin", "payroll"), +logger: logrus.WithField("plugin", "fees"),
35-38: Wrap RPC dial error to include target URLReturning the raw error drops the context of which URL failed.
-rpcClient, err := ethclient.Dial(cfg.RpcURL) -if err != nil { - return nil, err +rpcClient, err := ethclient.Dial(cfg.RpcURL) +if err != nil { + return nil, fmt.Errorf("ethclient dial %s: %w", cfg.RpcURL, err) }api/plugin.go (2)
63-66: Validate-then-sign pattern mixes concerns
ValidateProposedTransactionsis invoked before the message hash check.
If the hash check fails, expensive validation work was wasted. Consider swapping the two blocks to fail fast.
398-398: Usestrconv.Itoainstead offmt.Sprintffor int-to-stringMore idiomatic and allocates slightly less.
- fmt.Sprintf("%d", policy.PolicyVersion), + strconv.Itoa(policy.PolicyVersion),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
api/plugin.go(6 hunks)plugin/dca/dca.go(7 hunks)plugin/fees/config.go(1 hunks)plugin/fees/constraints.go(1 hunks)plugin/fees/fees.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- plugin/fees/constraints.go
🚧 Files skipped from review as they are similar to previous changes (1)
- plugin/dca/dca.go
🧰 Additional context used
🧬 Code Graph Analysis (3)
plugin/fees/config.go (1)
plugin/fees/constraints.go (1)
PLUGIN_TYPE(3-3)
plugin/fees/fees.go (3)
storage/db.go (1)
DatabaseStorage(14-41)plugin/fees/config.go (1)
PluginConfig(11-24)internal/types/keysign.go (1)
PluginKeysignRequest(7-12)
api/plugin.go (1)
internal/types/keysign.go (1)
PluginKeysignRequest(7-12)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
plugin/fees/fees.go (1)
48-55: Hard-coded recipe ID / version & leftover TODO
"vultisig-fees-0000"and version1are hard-coded. This will diverge once the plugin evolves.Clarify the TODO or derive these values from constants so they cannot get out of sync with constraints.go.
| if err := v.ReadInConfig(); err != nil { | ||
| return nil, fmt.Errorf("failed to read config: %w", err) | ||
| } |
There was a problem hiding this comment.
Differentiate between “file not found” and real config parse errors
v.ReadInConfig() returns an error when the config file is missing.
Right now any error aborts startup, even when users intend to rely solely on environment variables. Handle viper.ConfigFileNotFoundError explicitly so env-only setups still work.
-if err := v.ReadInConfig(); err != nil {
- return nil, fmt.Errorf("failed to read config: %w", err)
+if err := v.ReadInConfig(); err != nil {
+ // allow env-only configuration
+ if _, ok := err.(viper.ConfigFileNotFoundError); !ok {
+ return nil, fmt.Errorf("failed to read config: %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.
| if err := v.ReadInConfig(); err != nil { | |
| return nil, fmt.Errorf("failed to read config: %w", err) | |
| } | |
| if err := v.ReadInConfig(); err != nil { | |
| // allow env-only configuration | |
| if _, ok := err.(viper.ConfigFileNotFoundError); !ok { | |
| return nil, fmt.Errorf("failed to read config: %w", err) | |
| } | |
| } |
🤖 Prompt for AI Agents
In plugin/fees/config.go around lines 42 to 44, the current code treats all
errors from v.ReadInConfig() as fatal, including when the config file is
missing. Modify the error handling to explicitly check if the error is a
viper.ConfigFileNotFoundError and ignore it in that case, allowing the program
to continue when users rely only on environment variables. For other errors,
keep returning the wrapped error as before.
| if config.Gas.LimitMultiplier <= 0 { | ||
| return nil, errors.New("gas limit multiplier must be positive") | ||
| } |
There was a problem hiding this comment.
Validate Gas.PriceMultiplier as well
PriceMultiplier can silently be zero or negative, leading to an under-priced transaction and inevitable on-chain failure.
-if config.Gas.LimitMultiplier <= 0 {
- return nil, errors.New("gas limit multiplier must be positive")
+if config.Gas.LimitMultiplier <= 0 {
+ return nil, errors.New("gas limit multiplier must be positive")
+}
+if config.Gas.PriceMultiplier <= 0 {
+ return nil, errors.New("gas price multiplier must be positive")
}📝 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 config.Gas.LimitMultiplier <= 0 { | |
| return nil, errors.New("gas limit multiplier must be positive") | |
| } | |
| if config.Gas.LimitMultiplier <= 0 { | |
| return nil, errors.New("gas limit multiplier must be positive") | |
| } | |
| if config.Gas.PriceMultiplier <= 0 { | |
| return nil, errors.New("gas price multiplier must be positive") | |
| } |
🤖 Prompt for AI Agents
In plugin/fees/config.go around lines 58 to 60, add a validation check for
config.Gas.PriceMultiplier similar to the existing check for LimitMultiplier.
Ensure that PriceMultiplier is greater than zero, and if not, return an error
indicating that the gas price multiplier must be positive to prevent
under-priced transactions and on-chain failures.
| return &FeePlugin{ | ||
| db: db, | ||
| rpcClient: rpcClient, | ||
| logger: logrus.WithField("plugin", "payroll"), | ||
| config: cfg, | ||
| }, nil |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remember to close the ethclient.Client
ethclient.Client holds open connections; without a Close() the process leaks file descriptors.
Expose a Close() on FeePlugin (implement io.Closer) or close it in caller.
🤖 Prompt for AI Agents
In plugin/fees/fees.go around lines 40 to 45, the ethclient.Client instance
assigned to FeePlugin is not closed, causing resource leaks. Implement a Close()
method on FeePlugin that calls Close() on the ethclient.Client to release
resources properly. Ensure FeePlugin implements io.Closer and update callers to
invoke this Close() method when done using the plugin.
Fixes breaking changes introduced in verifier
Summary by CodeRabbit