MinExecWindow for policy#141
Conversation
WalkthroughThe changes update the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PayrollPlugin
participant Rule
participant TransactionProposer
User->>PayrollPlugin: ProposeTransactions()
PayrollPlugin->>Rule: Get recipient and amount (single)
Rule-->>PayrollPlugin: Return recipient, amount
PayrollPlugin->>TransactionProposer: Propose transaction for recipient and amount
TransactionProposer-->>PayrollPlugin: Transaction proposed
PayrollPlugin-->>User: Return result
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 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 (
|
![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
📜 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 (2)
go.mod(1 hunks)plugin/payroll/transaction.go(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
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/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: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.
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.
plugin/payroll/transaction.go (5)
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: #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: 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: #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/payroll/transaction.go (1)
internal/types/keysign.go (1)
PluginKeysignRequest(7-12)
⏰ 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 (3)
go.mod (1)
23-23: LGTM - Dependency update aligns with payroll plugin changes.The recipes dependency update appears coordinated with the payroll plugin changes that remove whitelist constraint support.
plugin/payroll/transaction.go (2)
203-206: LGTM - Function call updated correctly.The change from
RuleToRecipientsAndAmounttoRuleToRecipientAndAmountis consistent with the new single-recipient design, and the return value handling is updated appropriately.
429-463: LGTM - Function correctly refactored for single recipient.The function has been properly refactored to:
- Handle single recipient instead of multiple recipients
- Only accept FIXED constraint types (removing whitelist support)
- Maintain proper error handling and validation
- Return appropriate values for the new design
The implementation aligns perfectly with the PR objective of moving away from whitelist-based multiple recipients.
RaghavSood
left a comment
RaghavSood
left a comment
There was a problem hiding this comment.
LGTM, pending the recipes merge and rename for min exec
2 participants
I realised that having
whitelistfor recipients is not correct, since we can't configure different amount for each recipientRef: vultisig/recipes#47
Summary by CodeRabbit