refactor: remove policy field and update policy handling in DCA and P…#71
Conversation
WalkthroughThis change removes the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PluginServer
participant Database
Client->>PluginServer: POST /create_policy (no policy field)
PluginServer->>Database: Insert policy (without policy JSON)
Database-->>PluginServer: Confirmation
PluginServer-->>Client: Policy created
Client->>PluginServer: Request signature verification
PluginServer->>PluginServer: Concatenate key fields (Recipe, PublicKey, etc.)
PluginServer->>PluginServer: Verify signature with concatenated message
PluginServer-->>Client: Verification result
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
🪧 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
A refactor to remove the policy JSONB column from plugin_policies and centralize policy content in the recipe field, updating storage, migrations, scripts, plugin code, and the API accordingly.
- Drop the DB
policycolumn and strip out JSON marshaling/unmarshaling in the Postgres backend - Update dev scripts and plugin code to defer parsing of
policy.Recipe(currently stubbed with TODOs) - Adjust API signature logic to use the recipe-based message format and bump dependency versions
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| storage/postgres/policy.go | Removed raw JSON handling for policy, now uses recipe |
| storage/postgres/migrations/20250527071907_*.sql | Migration to drop/add the policy column |
| scripts/dev/create_payroll_policy/main.go | Commented out obsolete policy.Policy assignment |
| scripts/dev/create_dca_policy/main.go | Commented out obsolete policy.Policy assignment |
| plugin/payroll/transaction.go | Removed JSON unmarshal of policy.Policy, added TODOs |
| plugin/payroll/policy.go | Removed JSON unmarshal of policy.Policy, added TODOs |
| plugin/dca/dca.go | Removed JSON unmarshal of policy.Policy, added TODOs |
| internal/scheduler/scheduler.go | Switched unmarshal from policy.Policy to policy.Recipe |
| api/plugin.go | Updated SignPluginMessages and policyToMessageHex |
| go.mod | Bumped several dependency versions |
Comments suppressed due to low confidence (2)
scripts/dev/create_dca_policy/main.go:101
- The
Policyfield has been removed from the struct. You need to mappolicyBytesintopolicy.Recipeso the DCA policy is passed through correctly.
policy.Policy = policyBytes
api/plugin.go:73
- The error message prints the entire struct (
req.Messages[0]) instead of itsHash. Update the format to usereq.Messages[0].Hashfor thegotvalue.
return fmt.Errorf("message hash does not match transaction hash. expected %s, got %s", txHash, req.Messages[0])
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (3)
scripts/dev/create_payroll_policy/main.go (1)
124-130: 💡 Verification agent🧩 Analysis chain
Verify that servers can handle missing Policy field.
The
policy.Policyfield assignment has been commented out, but thepolicyBytesvariable is still created and unused. This change could cause issues if the verifier or plugin servers expect the Policy field to be populated.Consider removing the unused
policyBytesvariable if it's no longer needed:- policyBytes, err := json.Marshal(payrollPolicy) - if err != nil { - panic(err) - } - - fmt.Println("Payroll policy", string(policyBytes)) - // policy.Policy = policyBytes + policyJson, _ := json.Marshal(payrollPolicy) + fmt.Println("Payroll policy", string(policyJson))
🏁 Script executed:
#!/bin/bash # Check if there are any server-side handlers that still expect the Policy field echo "Searching for Policy field usage in server handlers..." # Look for Policy field access in HTTP handlers rg -A 5 -B 5 "\.Policy" --type go # Look for JSON unmarshaling that might expect Policy field rg -A 5 -B 5 "json\.Unmarshal.*Policy" --type goLength of output: 13168
Remove unused
policyBytesand commented-out assignmentWe searched the Go services (including HTTP handlers and JSON tags) and found no references to a
Policybyte‐slice field—onlyPolicyIDandPolicyVersion. It’s safe to drop the commented assignment, but thepolicyBytesvariable is now unused and should be either removed or repurposed for logging.Locations to update:
scripts/dev/create_payroll_policy/main.go(around lines 124–130)Suggested diff:
-scripts/dev/create_payroll_policy/main.go:124 - policyBytes, err := json.Marshal(payrollPolicy) - if err != nil { - panic(err) - } - fmt.Println("Payroll policy", string(policyBytes)) - // policy.Policy = policyBytes +scripts/dev/create_payroll_policy/main.go:124 + policyJSON, err := json.Marshal(payrollPolicy) + if err != nil { + panic(err) + } + fmt.Println("Payroll policy", string(policyJSON))Or, if you don’t need the JSON output for debugging, remove the marshalling block entirely.
plugin/payroll/transaction.go (1)
82-88:⚠️ Potential issuePotential panic on empty
txs
signRequest := txs[0]will panic when there are no recipients (see earlier issue).
Guard before accessing:if len(txs) == 0 { return nil, fmt.Errorf("no transactions generated") }plugin/dca/dca.go (1)
94-106:⚠️ Potential issue
dcaPolicynever parsed – leads to incorrect chainID & validation errors
chainID, ok := new(big.Int).SetString(dcaPolicy.ChainID, 10)operates on an empty string becausedcaPolicyis never filled. Similar issues appear throughout the file (price checks, token IDs, etc.).Parsing the recipe is mandatory before any validation or tx generation.
♻️ Duplicate comments (2)
plugin/dca/dca.go (2)
289-291:ProposeTransactionsuses empty policy – will always error or panic
totalAmountandtotalOrdersparse empty strings, inevitably returning errors and preventing proposal generation. Same root cause: unparsed recipe.
376-378:ValidateProposedTransactionssuffers from the same uninitialized struct issueAll policy checks below rely on empty
dcaPolicy; validation will incorrectly fail.
🧹 Nitpick comments (4)
storage/postgres/policy.go (1)
18-33: Column order coupling – consider naming columns explicitly inScanThe
SELECTlist andScanarguments rely on positional order. If a future migration re-orders columns the code will silently mis-map values.A safer pattern is to give each selected column an alias that matches the struct field order:
SELECT id, public_key, plugin_id, plugin_version, policy_version, signature, active, recipe FROM plugin_policies WHERE id = $1;and keep the same order in
Scan, or usepgx.RowToStructByName.Not mandatory, but lowers maintenance risk.
plugin/payroll/transaction.go (2)
44-54: Indexing parallel slices is brittle – prefer struct of recipient+token
payrollPolicy.ChainID[i],payrollPolicy.TokenID[i]assume all three slices are the same length and aligned.
Consider modelling each payment as a struct{ChainID, TokenID, Recipient}and looping over that slice to avoid alignment bugs.
60-74: Verify newKeysignMessageconstruction – field types & duplication
Chain: vcommon.Ethereum– confirm that the type matchesKeysignMessage.Chain(string vs enum).- You set
PolicyIDtwice: once inside the embeddedKeysignRequest, again in the outerPluginKeysignRequest. Duplication invites inconsistencies.- Removed
DerivePath/IsECDSA; ensure downstream services no longer rely on them.api/plugin.go (1)
72-74: Good change, but fix the error message for consistency.The hash comparison logic is correct and aligns with the updated message structure. However, the error message still references
req.Messages[0]which may now display the entire struct instead of just the hash.Consider updating the error message for clarity:
- if txHash != req.Messages[0].Hash { - return fmt.Errorf("message hash does not match transaction hash. expected %s, got %s", txHash, req.Messages[0]) - } + if txHash != req.Messages[0].Hash { + return fmt.Errorf("message hash does not match transaction hash. expected %s, got %s", txHash, req.Messages[0].Hash) + }
📜 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 (10)
api/plugin.go(3 hunks)go.mod(2 hunks)internal/scheduler/scheduler.go(1 hunks)plugin/dca/dca.go(5 hunks)plugin/payroll/policy.go(3 hunks)plugin/payroll/transaction.go(3 hunks)scripts/dev/create_dca_policy/main.go(0 hunks)scripts/dev/create_payroll_policy/main.go(1 hunks)storage/postgres/migrations/20250527071907_remove_policy_field.sql(1 hunks)storage/postgres/policy.go(4 hunks)
💤 Files with no reviewable changes (1)
- scripts/dev/create_dca_policy/main.go
🔇 Additional comments (5)
go.mod (1)
8-8:Details
✅ Verification successful
Verify security and validity of updated dependencies.
The dependency updates include critical cryptographic libraries and Ethereum-related packages. Ensure these versions are secure and free from known vulnerabilities.
Also applies to: 21-21, 29-32, 35-35
🏁 Script executed:
#!/bin/bash # Check for security advisories for the main updated dependencies echo "Checking security advisories for updated dependencies..." for pkg in "ethereum/go-ethereum" "consensys/gnark-crypto" "crate-crypto/go-eth-kzg"; do echo "=== Checking $pkg ===" gh api graphql -f query=" { securityVulnerabilities(first: 5, ecosystem: GO, package: \"$pkg\") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }" 2>/dev/null || echo "Unable to check security advisories for $pkg" doneLength of output: 1800
🌐 Web query:
Are there any known security vulnerabilities in ethereum/go-ethereum v1.15.11?💡 Result:
As of May 27, 2025, there are no known security vulnerabilities in Ethereum's go-ethereum (Geth) version 1.15.11. The latest release, v1.15.11, is a maintenance update that addresses issues with log and transaction indexing. Notably, it resolves a stall condition in
eth_getTransactionByHashand fixes two bugs in the new log indexer. Upgrading to this release is recommended to improve RPC performance. (newreleases.io)Previous vulnerabilities, such as CVE-2025-24883, which allowed a remote attacker to perform a denial of service (DoS) attack via specially crafted p2p messages, were addressed in earlier versions. Specifically, this vulnerability was fixed in version 1.14.13. (tenable.com)
For the most current information on security advisories, you can refer to the official Ethereum GitHub repository's security advisories page. (github.com)
Stock market information for Ethereum (ETH)
- Ethereum is a crypto in the CRYPTO market.
- The price is 2591.74 USD currently with a change of 7.79 USD (0.00%) from the previous close.
- The intraday high is 2599.37 USD and the intraday low is 2526.56 USD.
Citations:
- 1: https://newreleases.io/project/github/ethereum/go-ethereum/release/v1.15.11?utm_source=openai
- 2: https://www.tenable.com/cve/CVE-2025-24883?utm_source=openai
- 3: https://github.com/ethereum/go-ethereum/security/advisories?utm_source=openai
No security vulnerabilities found in updated dependencies
We checked the main updated Go modules via GitHub’s Security Advisories API and public release notes:
- ethereum/go-ethereum v1.15.11: no open CVEs (maintenance update with log-indexing and RPC fixes)
- consensys/gnark-crypto (latest commit): no reported vulnerabilities
- crate-crypto/go-eth-kzg v1.3.0: no known security advisories
Please continue to monitor the upstream security pages (e.g. github.com/ethereum/go-ethereum/security/advisories) for future updates.
storage/postgres/migrations/20250527071907_remove_policy_field.sql (1)
1-11:Details
❓ Verification inconclusive
Well-structured migration for policy field removal.
The migration properly removes the
policycolumn with appropriate safety checks usingIF EXISTSandIF NOT EXISTSclauses. The DOWN migration correctly restores the column with proper JSONB type and default value.
🏁 Script executed:
#!/bin/bash # Verify that all database queries have been updated to remove policy column references echo "Checking for remaining references to policy column in database queries..." # Look for SQL queries that might still reference the policy column rg -i "select.*policy|policy.*from|insert.*policy|update.*policy" --type go # Look for struct fields that might map to the policy column rg "Policy.*\`.*policy\`|policy.*\`.*Policy\`" --type goLength of output: 5253
Confirm removal of
policycolumn referencesThe migration correctly drops the
policycolumn (and restores it in the Down step) with safeIF EXISTS/IF NOT EXISTSguards. However, automated grep for SQL references was inconclusive—matches includepolicy_versionand method names rather than the JSONB column itself. Please manually verify that no code still touches the dropped column:• storage/postgres/policy.go – inspect INSERT/UPDATE queries for stray
policyreferences
• storage/db.go – methods InsertPluginPolicyTx, UpdatePluginPolicyTx
• service/policy.go – InsertPolicy and UpdatePolicy flows
• storage/postgres/time_trigger.go – joins or filters onpolicy_id
• plugin/dca/dca.go – calls to UpdatePluginPolicyTx
• internal/scheduler/scheduler.go – UpdateTriggerStatus
• api/plugin.go & api/server.go – endpoints handling plugin policiesplugin/payroll/policy.go (1)
68-70:Details
❌ Incorrect review comment
Destination address compared to
TokenIDlooks wrong
txDestination(the contract or recipient address the TX is sent to) is compared withpayrollPolicy.TokenID[i].
From the naming this looks like a token contract address, not the recipient’s address. The in-code TODO confirms the doubt.Please re-confirm the business rule. Most ERC-20 payroll flows:
to= token contract- ABI call arg 0 = recipient
If so, the check here should probably verify
recipientAddressrather thantokenID, or at least comparetxDestinationto a single contract address, not one per recipient.
🏁 Script executed:
#!/bin/bash # Locate the PayrollPolicy struct definition and TokenID field grep -R "type PayrollPolicy" -n plugin/payroll/policy.go grep -R "TokenID" -n plugin/payroll/policy.go # Show the first 200 lines for context (including function containing lines 68-70) sed -n '1,200p' plugin/payroll/policy.goLength of output: 5126
Comparing
txDestinationtoTokenIDis intentionalThe
PayrollPolicystruct defines a one-to-one mapping betweenTokenID[i]andRecipients[i](enforced by thelen(TokenID) == len(Recipients)check). In an ERC-20 payroll flow, you do indeed need to confirm that the transaction’stofield matches the expected token contract (per recipient) before unpacking and validating the recipient argument.The lingering
// todo : why we compare to tokenId…comment is now outdated and can be removed or updated to explain that this check ensures the TX targets the correct token contract for each recipient.Likely an incorrect or invalid review comment.
api/plugin.go (2)
324-324: Good simplification of the function signature.Removing the unused
updateparameter simplifies the API and aligns with the new deterministic policy message generation approach.
372-386: Excellent refactor to deterministic policy message generation.This change significantly improves the policy signature verification approach:
Benefits:
- Deterministic: Same policy always produces the same message bytes, eliminating JSON serialization inconsistencies
- Secure: Validates against delimiter injection attacks by rejecting fields containing "#"
- Simplified: Removes complexity of JSON marshaling and hex encoding
- Minimal: Only includes essential fields needed for signature verification
Security Analysis:
- The delimiter "#" is unlikely to appear in legitimate policy fields
- Input validation prevents signature tampering via delimiter injection
- Direct byte return eliminates encoding/decoding vulnerabilities
The field selection (Recipe, PublicKey, PolicyVersion, PluginVersion) appropriately covers the core policy identity without including mutable metadata.
| } | ||
|
|
||
| if err := json.Unmarshal(policy.Policy, &policySchedule); err != nil { | ||
| if err := json.Unmarshal([]byte(policy.Recipe), &policySchedule); err != nil { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify Recipe field contains expected schedule structure.
The change from policy.Policy to policy.Recipe for JSON unmarshaling aligns with the policy field removal. However, ensure that the Recipe field contains the schedule information in the expected JSON structure.
Additionally, consider adding validation to ensure the Recipe field contains valid schedule JSON:
if err := json.Unmarshal([]byte(policy.Recipe), &policySchedule); err != nil {
- return nil, fmt.Errorf("failed to parse policy schedule: %w", err)
+ return nil, fmt.Errorf("failed to parse policy schedule from recipe: %w", err)
}🏁 Script executed:
#!/bin/bash
# Verify that Recipe field is properly populated and contains schedule information
echo "Checking Recipe field usage and structure..."
# Look for where Recipe field is set/populated
rg -A 10 -B 5 "Recipe.*=" --type go
# Look for Recipe field in struct definitions
rg -A 5 -B 5 "Recipe.*string|Recipe.*\[\]byte" --type go
# Check for any remaining uses of policy.Policy field
rg -A 5 -B 5 "policy\.Policy" --type goLength of output: 4629
Verify Recipe JSON Structure and Update Error Handling
I’ve confirmed that:
storage/postgres/policy.gopopulatespolicy.Recipe(no remainingpolicy.Policyreferences).vtypes.PluginPolicy.Recipeis defined as a string and passed throughapi/plugin.go.- In
internal/scheduler/scheduler.go, you correctly unmarshalpolicy.RecipeintopolicySchedule.
Recommended fixes:
- Improve the error message when unmarshaling the recipe, to clarify the source.
- (Optional) Validate that
policySchedule.Scheduleis non-nil before accessing its fields.
Suggested diff in internal/scheduler/scheduler.go:
if err := json.Unmarshal([]byte(policy.Recipe), &policySchedule); err != nil {
- return nil, fmt.Errorf("failed to parse policy schedule: %w", err)
+ return nil, fmt.Errorf("failed to parse schedule from recipe: %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 := json.Unmarshal([]byte(policy.Recipe), &policySchedule); err != nil { | |
| if err := json.Unmarshal([]byte(policy.Recipe), &policySchedule); err != nil { | |
| return nil, fmt.Errorf("failed to parse schedule from recipe: %w", err) | |
| } |
🤖 Prompt for AI Agents
In internal/scheduler/scheduler.go at line 194, improve the error handling for
unmarshaling policy.Recipe by enhancing the error message to specify that the
failure occurred while parsing the Recipe JSON. Additionally, after
unmarshaling, add a validation step to check that the resulting
policySchedule.Schedule is not nil before accessing its fields, returning a
clear error if the schedule data is missing or invalid.
RaghavSood
left a comment
RaghavSood
left a comment
There was a problem hiding this comment.
LGTM
Thanks for the todos
…ayroll
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores