⚡️ Stable version of fees and soft delete implemented#139
Conversation
WalkthroughThe updates introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant API
participant DB
User->>API: DELETE /plugin/policy/{id}
API->>DB: Update plugin_policies set deleted=true where id={id}
API->>User: 200 OK { "id": "{id}" }
sequenceDiagram
participant FeePlugin
participant FeeConfig
participant EthereumSDK
FeePlugin->>FeeConfig: Get ChainId
FeePlugin->>EthereumSDK: Initialize with ChainId
sequenceDiagram
participant FeePlugin
participant EthereumTx
participant Signer
FeePlugin->>EthereumTx: Decode unsigned payload
FeePlugin->>Signer: Get transaction hash
Signer->>FeePlugin: Return hash
FeePlugin->>FeePlugin: SHA-256 hash of transaction hash
FeePlugin->>Signer: Send base64-encoded hash for signing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (9)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 implements soft delete functionality for plugin policies and stabilizes the fee plugin transaction handling. The changes introduce database-level constraints to prevent updates to deleted policies while ensuring proper transaction signing.
- Implements soft delete pattern for plugin_policies table with database triggers for data integrity
- Updates fee plugin to use proper Ethereum transaction hashing and base64 encoding for signing
- Changes API response for policy deletion to return JSON instead of empty response
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| storage/postgres/schema/schema.sql | Adds deleted column and database triggers for soft delete functionality |
| storage/postgres/migrations/plugin/20250721200403_delete_policy_constraints.sql | Migration implementing soft delete with database constraints |
| storage/postgres/policy.go | Updates delete operation to use soft delete instead of hard delete |
| plugin/fees/transaction.go | Improves transaction signing with proper Ethereum hash calculation |
| go.mod | Updates verifier dependency version |
| fee.worker.example.json | Adds configuration option for setup messages |
| api/plugin.go | Changes delete API response from 204 to 200 with JSON body |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
api/plugin.go (1)
183-186: API response change may break existing clientsChanging from 204 No Content to 200 OK with a JSON response body is a breaking change for API clients. Clients expecting a 204 status may fail when receiving 200.
Consider:
- Documenting this breaking change in the API changelog
- Versioning the API endpoint if backward compatibility is needed
- Ensuring all API clients are updated to handle the new response format
storage/postgres/schema/schema.sql (1)
178-181: Missing partial index for fast look-ups on non-deleted active policiesTypical queries will filter on
WHERE deleted = false AND active = true.
Creating a partial index greatly speeds these reads and keeps bloat down:CREATE INDEX idx_plugin_policies_active_not_deleted ON plugin_policies(id) WHERE active = true AND deleted = false;Consider adding this in a follow-up migration.
📜 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)
api/plugin.go(1 hunks)fee.worker.example.json(1 hunks)go.mod(1 hunks)plugin/fees/transaction.go(5 hunks)storage/postgres/migrations/plugin/20250721200403_delete_policy_constraints.sql(1 hunks)storage/postgres/policy.go(1 hunks)storage/postgres/schema/schema.sql(3 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
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)`.
go.mod (2)
Learnt from: RaghavSood
PR: #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: #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.
fee.worker.example.json (3)
Learnt from: johnnyluo
PR: #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: #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: garry-sharp
PR: #113
File: fee.worker.example.json:9-9
Timestamp: 2025-07-03T15:54:18.797Z
Learning: In configuration files, local_party_prefix values can contain hexadecimal digit patterns like "feee" which are intentional, not typos.
storage/postgres/policy.go (2)
Learnt from: webpiratt
PR: #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: webpiratt
PR: #125
File: plugin/payroll/policy.go:160-160
Timestamp: 2025-07-09T22:23:17.348Z
Learning: In plugin/payroll/policy.go, the "token" case in the checkRule method intentionally does not call validateToken() as this is a temporary implementation that will be replaced with generic schema validation. The missing validation call is expected behavior during this transition period.
plugin/fees/transaction.go (10)
Learnt from: webpiratt
PR: #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: #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: garry-sharp
PR: #117
File: plugin/fees/policy.go:46-47
Timestamp: 2025-07-04T10:47:47.927Z
Learning: For the fee plugin in plugin/fees/policy.go, the resource validation and recipe specification are intentionally configured to accept only USDC transfers ("ethereum.usdc.transfer"), not general ERC20 transfers ("ethereum.erc20.transfer"), as fees are only collected in USDC.
Learnt from: webpiratt
PR: #125
File: plugin/payroll/transaction.go:186-197
Timestamp: 2025-07-09T22:22:36.651Z
Learning: In plugin/payroll/transaction.go, the getTokenID function is designed to return evm.ZeroAddress.Hex() when no "token" parameter constraint is found, rather than returning an error. This is expected behavior according to the team's design intent, even though the token parameter is marked as required in the recipe specification.
Learnt from: webpiratt
PR: #96
File: plugin/payroll/transaction.go:178-183
Timestamp: 2025-06-18T18:28:19.759Z
Learning: In the payroll plugin, the Hash field in PluginKeysignRequest is intentionally set to the unsigned transaction hex (same as Message field) because computing a hash with empty V,R,S signature fields doesn't make sense, and it's documented as "not on-chain hash without signature". This is a deliberate placeholder approach.
Learnt from: webpiratt
PR: #126
File: plugin/fees/policy.go:26-26
Timestamp: 2025-07-10T20:41:44.025Z
Learning: The constant vtypes.PluginVultisigFees_feee in the vultisig/plugin project is correctly spelled with "feee" (including the extra 'e'). This is the actual constant name defined in the external dependency github.com/vultisig/verifier/types and should not be changed to "fee" singular, as this is how it's defined in the external package.
Learnt from: RaghavSood
PR: #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: #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: garry-sharp
PR: #117
File: plugin/fees/constraints.go:3-3
Timestamp: 2025-07-04T10:50:42.832Z
Learning: For the vultisig/plugin project, use "fee" (singular) throughout the codebase wherever possible, not "fees" (plural). This applies to all naming conventions including plugin types, variable names, function names, comments, and documentation.
Learnt from: webpiratt
PR: #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/migrations/plugin/20250721200403_delete_policy_constraints.sql (2)
Learnt from: webpiratt
PR: #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: #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.
storage/postgres/schema/schema.sql (1)
Learnt from: webpiratt
PR: #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).
🔇 Additional comments (5)
fee.worker.example.json (1)
13-14: Configuration property addition looks goodThe new
do_setup_msgproperty is properly added with correct JSON syntax.go.mod (1)
23-23: Verified compatibility of updated verifier dependency – no breaking changes detectedThe commit history between
de5b88138d5cand058f80801ad2only includes UI, build, and configuration tweaks (marketplace v2, styling, vite config fixes, etc.) with no alterations to the transaction signing API or related interfaces. You can safely merge this update.storage/postgres/policy.go (1)
160-178: Soft delete implementation validatedThe soft delete for
plugin_policiesis correctly implemented and verified:
- The
deletedboolean column (default false) exists in both schema and migration (20250721200403_delete_policy_constraints.sql).- Triggers are in place:
set_policy_inactive_on_deletesetsactive = falsewhendeleted = true.prevent_update_if_policy_deletedblocks any updates oncedeletedis true.- Manual removal of
time_triggersis required (there’s no ON DELETE CASCADE on that table).- Related
fee_runrecords referenceplugin_policies(id)ON DELETE CASCADE, but since we’re only marking rows as deleted, those entries remain—preserving audit/history data.Please confirm this retention strategy aligns with your data-retention requirements. All checks pass.
plugin/fees/transaction.go (2)
129-135: Good security improvement: signing transaction hash instead of raw bytesThe change to decode the transaction and compute its proper hash before signing is a security best practice. This prevents transaction malleability attacks.
150-161: Double hashing pattern confirmed and intentional
The SHA256 double-hash approach (Message = base64(txHash), Hash = base64(sha256(txHash)), HashFunction = SHA256) is already used in both plugin/fees/transaction.go and plugin/payroll/transaction.go. No further changes are required.
c847827 to
847c555
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
storage/postgres/schema/schema.sql (2)
25-35: Remove redundant check inside trigger functionBecause the trigger is already declared with
WHEN (NEW.deleted = true), the internalIF NEW.deleted = true THEN … END IF;is never false.
Dropping the extra conditional trims a branch that will never be taken and makes the intent clearer.BEGIN - IF NEW.deleted = true THEN - RAISE EXCEPTION 'Cannot insert a deleted policy'; - END IF; + RAISE EXCEPTION 'Cannot insert a deleted policy'; RETURN NEW; END;
189-193: Trigger firing order is undefined – ensure deterministic behaviourAll three policy triggers fire BEFORE UPDATE. PostgreSQL executes them in alphabetical name order unless an explicit
FOLLOWS/PRECEDESclause is used.
If future edits add more logic, an unexpected order (e.g.trg_update_plugin_policies_updated_atrunning beforetrg_set_policy_inactive_on_delete) could persist stale timestamps or raise false exceptions.Consider adding explicit ordering directives:
CREATE TRIGGER trg_set_policy_inactive_on_delete BEFORE UPDATE ON plugin_policies FOR EACH ROW WHEN (OLD.deleted = false AND NEW.deleted = true) EXECUTE FUNCTION set_policy_inactive_on_delete() FOLLOWS trg_prevent_update_if_policy_deleted;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
api/plugin.go(1 hunks)fee.worker.example.json(1 hunks)plugin/fees/transaction.go(5 hunks)storage/postgres/migrations/plugin/20250721200403_delete_policy_constraints.sql(1 hunks)storage/postgres/policy.go(1 hunks)storage/postgres/schema/schema.sql(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- fee.worker.example.json
🚧 Files skipped from review as they are similar to previous changes (4)
- storage/postgres/policy.go
- plugin/fees/transaction.go
- api/plugin.go
- storage/postgres/migrations/plugin/20250721200403_delete_policy_constraints.sql
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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)`.
storage/postgres/schema/schema.sql (1)
Learnt from: webpiratt
PR: #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).
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
5469dae to
c0e889d
Compare
RaghavSood
left a comment
RaghavSood
left a comment
There was a problem hiding this comment.
Need to fix the migrations to use the system flow from verifier as mentioned in vultisig/verifier#274
9809c90 to
6073c74
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
storage/postgres/schema/schema.sql (1)
97-109:updated_atcolumn never advances forplugin_policiesThe table adds
updated_at(line 107) but unlikefee_run(line 195) there is noBEFORE UPDATEtrigger to callupdate_updated_at_column().
Consequently any change —including the soft-delete path—leaves the timestamp stale, breaking audit expectations.+-- keep naming consistent with other tables +CREATE TRIGGER "trg_update_plugin_policies_updated_at" +BEFORE UPDATE ON "plugin_policies" +FOR EACH ROW +EXECUTE FUNCTION "public"."update_updated_at_column"();Also applies to: 189-195
🧹 Nitpick comments (1)
storage/postgres/schema/schema.sql (1)
107-109: Consider indexing thedeletedflagQueries that need “only active policies” will now add
WHERE deleted = false. A small, low-cardinality partial index helps:CREATE INDEX idx_plugin_policies_not_deleted ON plugin_policies(id) -- or any frequently-filtered column WHERE deleted = false;This keeps the hot path fast without bloating the index for rarely-touched deleted rows.
📜 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 (8)
api/plugin.go(1 hunks)fee.worker.example.json(1 hunks)go.mod(1 hunks)plugin/fees/config.go(4 hunks)plugin/fees/fees.go(1 hunks)plugin/fees/transaction.go(4 hunks)storage/postgres/policy.go(1 hunks)storage/postgres/schema/schema.sql(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- go.mod
🚧 Files skipped from review as they are similar to previous changes (6)
- fee.worker.example.json
- api/plugin.go
- plugin/fees/fees.go
- storage/postgres/policy.go
- plugin/fees/transaction.go
- plugin/fees/config.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: migration-test
Summary by CodeRabbit
New Features
Bug Fixes
Refactor