This repository was archived by the owner on Feb 8, 2026. It is now read-only.
use same recipe<>schema validation on verifier and plugin#126
Merged
Conversation
Contributor
WalkthroughThis update centralizes plugin policy validation by introducing a generic Changes
Sequence Diagram(s)sequenceDiagram
participant Plugin as Plugin (e.g., FeePlugin, PayrollPlugin)
participant InternalPlugin as internal/plugin
participant Engine as Engine
participant Schema as RecipeSchema
Plugin->>InternalPlugin: ValidatePluginPolicy(policyDoc, GetRecipeSpecification())
InternalPlugin->>Engine: Validate(policy, schema)
Engine-->>InternalPlugin: Validation result
InternalPlugin-->>Plugin: error or nil
Possibly related PRs
Suggested reviewers
Poem
✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (9)
go.mod(3 hunks)internal/plugin/plugin.go(1 hunks)plugin/dca/dca.go(1 hunks)plugin/fees/policy.go(2 hunks)plugin/payroll/constants.go(0 hunks)plugin/payroll/nonce.go(0 hunks)plugin/payroll/policy.go(2 hunks)plugin/payroll/transaction.go(1 hunks)service/policy.go(2 hunks)
💤 Files with no reviewable changes (2)
- plugin/payroll/constants.go
- plugin/payroll/nonce.go
🧰 Additional context used
🧠 Learnings (6)
📓 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: 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#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.
internal/plugin/plugin.go (3)
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#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: 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.
plugin/payroll/policy.go (9)
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: 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#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: 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: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.
Learnt from: webpiratt
PR: vultisig/plugin#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: 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: johnnyluo
PR: vultisig/plugin#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: webpiratt
PR: vultisig/plugin#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.
plugin/payroll/transaction.go (3)
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.
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#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.
go.mod (3)
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: johnnyluo
PR: vultisig/plugin#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.
plugin/fees/policy.go (7)
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#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: 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: 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: 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.
Learnt from: webpiratt
PR: vultisig/plugin#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.
Learnt from: webpiratt
PR: vultisig/plugin#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.
🧬 Code Graph Analysis (3)
internal/plugin/plugin.go (1)
service/policy.go (1)
Policy(17-23)
plugin/payroll/policy.go (1)
internal/plugin/plugin.go (1)
ValidatePluginPolicy(13-30)
plugin/payroll/transaction.go (1)
plugin/payroll/payroll.go (1)
PayrollPlugin(19-29)
⏰ 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
🔇 Additional comments (15)
service/policy.go (2)
5-5: LGTM! Adding the errors package for enhanced error checking.The import enables the improved error handling in the
handleRollbackmethod.
45-45: Excellent improvement to error handling logic.Suppressing log messages for
pgx.ErrTxClosedprevents unnecessary noise in logs since attempting to rollback an already-closed transaction is not a genuine error condition.plugin/dca/dca.go (1)
777-778: LGTM! Method signature updated to support centralized validation.The change from returning a value to a pointer aligns with the new
plugin.ValidatePluginPolicyfunction signature and maintains consistency across plugins.plugin/payroll/transaction.go (1)
355-356: LGTM! Consistent method signature update.The change to return a pointer instead of a value aligns with the centralized validation pattern and maintains consistency with other plugins.
internal/plugin/plugin.go (1)
13-30: Excellent centralization of policy validation logic.This function effectively consolidates validation steps that were previously duplicated across plugins:
- Base64 decoding with proper error context
- Protobuf unmarshaling with error handling
- Engine-based schema validation
The error wrapping provides clear context for debugging validation failures.
plugin/payroll/policy.go (2)
7-7: LGTM! Adding import for centralized validation.The import enables the use of the new centralized
ValidatePluginPolicyfunction.
53-53: Excellent refactoring to use centralized validation.Based on the retrieved learnings, this replaces the temporary validation implementation with the proper centralized approach, improving consistency across plugins while maintaining the same validation capabilities.
go.mod (5)
19-19: LGTM: Testify moved to direct dependencyMoving
testifyfrom indirect to direct dependency is appropriate given its likely increased usage in the codebase.
22-23: LGTM: Recipe and verifier dependency updatesThe version updates to newer commits for
recipesandverifierpackages align with the centralized validation refactoring.
25-26: LGTM: Sync and protobuf moved to direct dependenciesMoving
golang.org/x/syncandgoogle.golang.org/protobufto direct dependencies is appropriate as they're now used directly in the new centralized plugin validation logic.
90-90: LGTM: Protobuf reorganizationMoving
github.com/golang/protobufto indirect while promotinggoogle.golang.org/protobufto direct is consistent with using the newer protobuf library directly.
133-133: LGTM: New indirect dependencyAdding
github.com/stretchr/objxas an indirect dependency is consistent with the testify package reorganization.plugin/fees/policy.go (3)
4-4: LGTM: Internal plugin package importThe import of the internal plugin package is appropriate for accessing the centralized validation function.
19-19: LGTM: Centralized validation delegationThe refactoring to delegate policy validation to the generic
plugin.ValidatePluginPolicyfunction reduces code duplication and centralizes validation logic.
22-23: LGTM: Return pointer for consistencyChanging the return type to
*rtypes.RecipeSchemais consistent with similar changes in other plugins and follows Go best practices for potentially large structs.
webpiratt
commented
Jul 10, 2025
|
|
||
| func (p *PayrollPlugin) GetRecipeSpecification() rtypes.RecipeSchema { | ||
| return rtypes.RecipeSchema{ | ||
| func (p *PayrollPlugin) GetRecipeSpecification() *rtypes.RecipeSchema { |
Contributor
Author
There was a problem hiding this comment.
Why changed to pointer everywhere? Any proto generated structs should be used as pointers
webpiratt
enabled auto-merge
July 10, 2025 20:41
webpiratt
changed the title
This was referenced Jul 22, 2025
Merged
This was referenced Jul 29, 2025
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ref: #124
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores
Style