✨ Update recipe dependency and enhance fee plugin functionality with magic constants#143
Conversation
WalkthroughThe changes update dependency declarations in Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FeePlugin
participant Resolver
participant Database
User->>FeePlugin: ProposeTransactions(request)
FeePlugin->>Resolver: ResolveTreasuryAddress(magic_constant)
Resolver-->>FeePlugin: TreasuryAddress
FeePlugin->>FeePlugin: Validate token constraint
FeePlugin->>FeePlugin: Build transaction hex
FeePlugin->>User: Return signing request (with tx hex)
User->>FeePlugin: initSign(signing request)
FeePlugin->>Database: Mark fee run as sent (uuid.Nil)
FeePlugin-->>User: Acknowledge
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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 updates the fee plugin functionality to use magic constants for treasury addresses and enhances configuration handling. The changes modernize the recipe dependency system and improve error handling throughout the fee collection workflow.
- Updated
github.com/vultisig/recipesdependency to a newer version - Enhanced fee plugin to use magic constants for treasury recipient resolution instead of fixed addresses
- Modified policy creation to include proper schedule validation and configuration handling
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| go.mod | Updates recipes dependency version |
| service/policy.go | Adds schedule validation before trigger creation and includes configuration in recipe specification |
| plugin/fees/policy.go | Updates recipe specification to support magic constants and adds token parameter |
| plugin/fees/transaction.go | Refactors transaction proposal to use treasury resolver with magic constants and removes tx indexer dependency |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plugin/fees/transaction.go (1)
170-170: TODO acknowledged for fee run tracking.The placeholder
uuid.Nilwith TODO comment indicates this will be properly implemented in a follow-up PR.Would you like me to create an issue to track this TODO for the upcoming PR?
📜 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 (4)
go.mod(1 hunks)plugin/fees/policy.go(4 hunks)plugin/fees/transaction.go(6 hunks)service/policy.go(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: garry-sharp
PR: vultisig/plugin#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: vultisig/plugin#141
File: plugin/payroll/transaction.go:208-267
Timestamp: 2025-07-24T19:34:47.441Z
Learning: In the vultisig/plugin codebase, duplicate transaction prevention for the payroll plugin has been moved to the verifier side, so the local `IsAlreadyProposed` check in plugin/payroll/transaction.go is no longer needed. The validation flow has changed and is now handled by the verifier rather than within the plugin itself.
Learnt from: webpiratt
PR: vultisig/plugin#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: garry-sharp
PR: vultisig/plugin#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.
plugin/fees/policy.go (5)
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: #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.
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: 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.
service/policy.go (2)
Learnt from: webpiratt
PR: #141
File: plugin/payroll/transaction.go:208-267
Timestamp: 2025-07-24T19:34:47.441Z
Learning: In the vultisig/plugin codebase, duplicate transaction prevention for the payroll plugin has been moved to the verifier side, so the local IsAlreadyProposed check in plugin/payroll/transaction.go is no longer needed. The validation flow has changed and is now handled by the verifier rather than within the plugin itself.
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.
go.mod (3)
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: webpiratt
PR: #141
File: plugin/payroll/transaction.go:208-267
Timestamp: 2025-07-24T19:34:47.441Z
Learning: In the vultisig/plugin codebase, duplicate transaction prevention for the payroll plugin has been moved to the verifier side, so the local IsAlreadyProposed check in plugin/payroll/transaction.go is no longer needed. The validation flow has changed and is now handled by the verifier rather than within the plugin itself.
plugin/fees/transaction.go (10)
Learnt from: webpiratt
PR: #141
File: plugin/payroll/transaction.go:208-267
Timestamp: 2025-07-24T19:34:47.441Z
Learning: In the vultisig/plugin codebase, duplicate transaction prevention for the payroll plugin has been moved to the verifier side, so the local IsAlreadyProposed check in plugin/payroll/transaction.go is no longer needed. The validation flow has changed and is now handled by the verifier rather than within the plugin itself.
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/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.
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: #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: 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: 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: 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: 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: 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.
🧬 Code Graph Analysis (1)
plugin/fees/policy.go (1)
internal/plugin/plugin.go (1)
RecipeConfiguration(35-51)
⏰ 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
🔇 Additional comments (6)
go.mod (1)
24-24: LGTM!The dependency update aligns with the PR objectives to support magic constants functionality.
service/policy.go (1)
65-75: Good improvement to schedule validation.The explicit schedule retrieval and error handling prevents creating time triggers without valid schedules, making the policy creation more robust.
plugin/fees/policy.go (2)
47-47: Verify the resource path change aligns with requirements.The change from
"ethereum.usdc.transfer"to"ethereum.erc20.transfer"appears to contradict the fee plugin's design to only accept USDC transfers. While the newtokenparameter constraint can enforce USDC-only transfers, this broader resource path might cause confusion.Based on previous learnings, the fee plugin was intentionally configured to accept only USDC transfers, not general ERC20 transfers.
30-80: Configuration and parameter updates look good.The addition of recipe configuration and the shift to magic constants for recipient handling aligns well with the PR objectives. The required token parameter ensures proper validation of the USDC token address.
plugin/fees/transaction.go (2)
69-69: Ensure resource path change is intentional.The case statement now expects
"ethereum.erc20.transfer"instead of"ethereum.usdc.transfer". This change should be consistent with the policy specification and the fee plugin's USDC-only requirement.
92-104: Treasury resolution implementation looks good.The magic constant validation and treasury address resolution are properly implemented. The hardcoded "ethereum" and "usdc" parameters are appropriate for the fee plugin's current scope.
24b29bc to
3c1819a
Compare
3c1819a to
0dcd1d9
Compare
0dcd1d9 to
ec5f1ad
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
storage/postgres/schema/schema.sql (1)
31-40: Redundant check in triggertrg_prevent_update_if_policy_deleted.The trigger fires only when
old.deleted = true(triggerWHENclause), yet the body repeats the same condition (IF OLD.deleted = true THEN …). Dropping one of them would simplify the code without changing behaviour.CREATE TRIGGER "trg_prevent_update_if_policy_deleted" BEFORE UPDATE ON "plugin_policies" - FOR EACH ROW WHEN (("old"."deleted" = true)) + FOR EACH ROW EXECUTE FUNCTION "public"."prevent_update_if_policy_deleted"();and remove the
IF …guard inside the function, or vice-versa.
Tiny, but it keeps triggers easier to read.
📜 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 (4)
go.mod(3 hunks)plugin/fees/policy.go(4 hunks)plugin/fees/transaction.go(6 hunks)storage/postgres/schema/schema.sql(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- go.mod
- plugin/fees/transaction.go
- plugin/fees/policy.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: garry-sharp
PR: vultisig/plugin#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: vultisig/plugin#141
File: plugin/payroll/transaction.go:208-267
Timestamp: 2025-07-24T19:34:47.441Z
Learning: In the vultisig/plugin codebase, duplicate transaction prevention for the payroll plugin has been moved to the verifier side, so the local `IsAlreadyProposed` check in plugin/payroll/transaction.go is no longer needed. The validation flow has changed and is now handled by the verifier rather than within the plugin itself.
Learnt from: webpiratt
PR: vultisig/plugin#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: garry-sharp
PR: vultisig/plugin#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: 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.
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#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.
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.
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: migration-test
🔇 Additional comments (2)
storage/postgres/schema/schema.sql (2)
20-29: Trigger function looks good.The insert-guard correctly blocks rows marked as deleted from being created. No issues spotted.
42-51: Automatic de-activation on logical delete is a solid addition.
set_policy_inactive_on_deletecleanly enforcesactive = falsewheneverdeleted = true, avoiding inconsistencies at application level.
| "active" boolean DEFAULT true NOT NULL, | ||
| "created_at" timestamp with time zone DEFAULT "now"() NOT NULL, | ||
| "updated_at" timestamp with time zone DEFAULT "now"() NOT NULL | ||
| "updated_at" timestamp with time zone DEFAULT "now"() NOT NULL, | ||
| "deleted" boolean DEFAULT false NOT NULL | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
updated_at column is never refreshed – add an UPDATE trigger for plugin_policies.
You introduce the updated_at column (Line 102) but, unlike fee_run, there is no BEFORE UPDATE trigger that bumps the timestamp. As a result, updated_at will always equal created_at, defeating the purpose of the column and making audit trails unreliable.
Add a trigger re-using the already defined update_updated_at_column() helper:
+-- Keep the timestamp current on every row modification
+CREATE TRIGGER "update_plugin_policies_updated_at"
+ BEFORE UPDATE ON "plugin_policies"
+ FOR EACH ROW
+ EXECUTE FUNCTION "public"."update_updated_at_column"();Place it next to the other plugin_policies triggers (Lines 162-166) so it is obvious and executed in the same migration step.
Also applies to: 162-166
🤖 Prompt for AI Agents
In storage/postgres/schema/schema.sql around lines 100 to 104, the updated_at
column is defined but never updated on row modifications, so it remains equal to
created_at. To fix this, add a BEFORE UPDATE trigger on the plugin_policies
table that calls the existing update_updated_at_column() function. Place this
trigger definition near the other plugin_policies triggers around lines 162 to
166 to keep related changes together and ensure updated_at is refreshed
automatically on updates.
✨ Update recipe dependency and enhance fee plugin functionality with magic constants
github.com/vultisig/recipesingo.modandgo.sum.GetRecipeSpecificationmethod inpolicy.goto include configuration handling.ProposeTransactionsmethod intransaction.goto validate token and recipient constraints, ensuring they match expected values.CreatePolicymethod inpolicy.goto handle schedule retrieval and trigger creation more robustly.Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores