payroll: propose and validated transactions based on recipes#100
Conversation
WalkthroughThis change introduces support for vault storage and encryption secrets in the payroll plugin, updating configuration structures, plugin initialization, and transaction proposal logic. The payroll policy and transaction handling are refactored to use a recipe-driven approach, replacing manual transaction decoding and recipient management with generalized rule-based evaluation and proposal. Changes
Sequence Diagram(s)sequenceDiagram
participant Server/Worker
participant PayrollPlugin
participant VaultStorage
participant ChainRegistry
participant Engine
Server/Worker->>PayrollPlugin: NewPayrollPlugin(db, vaultStorage, ..., encryptionSecret)
PayrollPlugin->>VaultStorage: Store reference, use encryptionSecret
PayrollPlugin->>VaultStorage: GetVaultFromPolicy(policy, encryptionSecret)
VaultStorage-->>PayrollPlugin: Decrypted vault
PayrollPlugin->>ChainRegistry: Get chain by ID
PayrollPlugin->>Engine: Evaluate transaction recipe rules
Engine-->>PayrollPlugin: Evaluation result
PayrollPlugin-->>Server/Worker: Proposed transactions or validation 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
This PR transitions the payroll processing functionality from using the legacy PayrollPolicy to a recipe‐based approach for constructing and validating transactions. Key changes include:
- Removing the legacy create_payroll_policy script.
- Refactoring transaction proposal logic to extract data from recipes.
- Updating plugin policy validation and configuration to incorporate vault storage and encryption secret handling.
Reviewed Changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| scripts/dev/create_payroll_policy/main.go | Removal of the legacy payroll policy script. |
| plugin/payroll/transaction.go | Refactored transaction proposal logic to use recipe rules. |
| plugin/payroll/policy.go | Updated plugin policy validation to derive and evaluate recipe data. |
| plugin/payroll/payroll.go | Updated plugin initialization to include vault storage and secret. |
| go.mod | Updated dependency version for the verifier module. |
| common/vault.go | Added helper to retrieve and decrypt vault data from storage. |
| cmd/payroll/worker/* & cmd/payroll/server/* | Updated worker and server initialization to pass vault and secret. |
Comments suppressed due to low confidence (1)
plugin/payroll/transaction.go:250
- [nitpick] Adding inline comments here to explain the conversion from a recipe rule to recipient and amount data would improve clarity and long-term maintainability.
recipient, amountStr, err := RuleToRecipientAndAmount(rule)
| i := _i | ||
| recipient := _recipient | ||
|
|
||
| for _, rule := range recipe.Rules { |
There was a problem hiding this comment.
Consider whether failing the entire transaction proposal on a single rule parsing error is intended. If robustness is desired, you might aggregate errors or skip invalid rules instead of returning an error immediately.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
plugin/payroll/payroll.go (1)
30-38: Consider adding validation for vault storage parameter.The constructor validates the database parameter but not the vault storage. Consider adding similar validation for consistency.
func NewPayrollPlugin( db storage.DatabaseStorage, vaultStorage vault.Storage, baseConfigPath string, txIndexerService *tx_indexer.Service, client *asynq.Client, inspector *asynq.Inspector, encryptionSecret string, ) (*PayrollPlugin, error) { if db == nil { return nil, fmt.Errorf("database storage cannot be nil") } + if vaultStorage == nil { + return nil, fmt.Errorf("vault storage cannot be nil") + }
📜 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)
cmd/payroll/server/config.go(1 hunks)cmd/payroll/server/main.go(1 hunks)cmd/payroll/worker/config.go(1 hunks)cmd/payroll/worker/main.go(1 hunks)common/vault.go(1 hunks)go.mod(1 hunks)plugin/payroll/payroll.go(3 hunks)plugin/payroll/policy.go(4 hunks)plugin/payroll/transaction.go(7 hunks)scripts/dev/create_payroll_policy/main.go(0 hunks)
💤 Files with no reviewable changes (1)
- scripts/dev/create_payroll_policy/main.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (10)
go.mod (1)
23-23: LGTM: Dependency update aligns with new vault functionality.The verifier dependency update is consistent with the vault storage and encryption features being added to the payroll plugin.
cmd/payroll/worker/config.go (1)
25-25: LGTM: Configuration field added correctly.The
EncryptionSecretfield is properly tagged and follows the established configuration pattern. Ensure this secret is handled securely in deployment (environment variables, secret management systems).cmd/payroll/server/config.go (1)
26-26: LGTM: Consistent configuration field addition.The
EncryptionSecretfield matches the worker configuration structure. Same security handling recommendations apply.plugin/payroll/payroll.go (3)
12-12: LGTM: Import added for vault storage functionality.The vault import is correctly added to support the new functionality.
26-27: LGTM: Plugin fields added appropriately.The new fields follow Go naming conventions and are properly typed. The encryptionSecret will be stored in memory during plugin lifecycle, which is appropriate for this use case.
61-62: LGTM: Constructor properly assigns new fields.The new fields are correctly assigned in the constructor.
cmd/payroll/worker/main.go (1)
84-84: LGTM: Constructor call updated correctly.The plugin constructor call now properly passes the vault storage and encryption secret parameters, matching the updated signature in
plugin/payroll/payroll.go.cmd/payroll/server/main.go (1)
74-74: LGTM!The plugin initialization correctly passes the vault storage and encryption secret parameters, aligning with the new vault integration requirements.
plugin/payroll/policy.go (1)
32-35: ```shell
#!/bin/bashLocate all 'chain' package directories and inspect for registry and GetChain
echo "=== Chain package directories ==="
find . -type d -name chainfor DIR in $(find . -type d -name chain); do
echo
echo "=== Inspecting $DIR ==="
echo "-- Go files --"
find "$DIR" -maxdepth 1 -type f -name '*.go'echo "-- Registry map definitions (map[string]Chain) --"
rg -C2 "map\[string\]Chain" "$DIR" || echo " No map[string]Chain found in $DIR"echo "-- GetChain implementation --"
rg -C2 "^func GetChain" "$DIR" || echo " No 'func GetChain' declaration in $DIR"echo "-- GetChain lookup logic --"
rg -C2 "GetChain\(" "$DIR" || echo " No 'GetChain(' pattern in $DIR"
done</details> <details> <summary>plugin/payroll/transaction.go (1)</summary> `373-432`: **Well-structured recipe specification!** The `GetRecipeSpecification` method provides a comprehensive schema that clearly defines the plugin's capabilities and constraints. This recipe-driven approach significantly improves maintainability and flexibility compared to the previous hardcoded policy structure. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| func GetVaultFromPolicy(s vault.Storage, policy vtypes.PluginPolicy, encryptionSecret string) (*v1.Vault, error) { | ||
| vaultFileName := vcommon.GetVaultBackupFilename(policy.PublicKey, policy.PluginID.String()) | ||
| vaultContent, err := s.GetVault(vaultFileName) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get vault") | ||
| } | ||
|
|
||
| if vaultContent == nil { | ||
| return nil, fmt.Errorf("vault not found") | ||
| } | ||
|
|
||
| return vcommon.DecryptVaultFromBackup(encryptionSecret, vaultContent) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error messages to include more context.
The error messages should wrap the original errors and provide more context for better debugging.
Apply this diff to improve error handling:
func GetVaultFromPolicy(s vault.Storage, policy vtypes.PluginPolicy, encryptionSecret string) (*v1.Vault, error) {
vaultFileName := vcommon.GetVaultBackupFilename(policy.PublicKey, policy.PluginID.String())
vaultContent, err := s.GetVault(vaultFileName)
if err != nil {
- return nil, fmt.Errorf("failed to get vault")
+ return nil, fmt.Errorf("failed to get vault %s: %w", vaultFileName, err)
}
if vaultContent == nil {
- return nil, fmt.Errorf("vault not found")
+ return nil, fmt.Errorf("vault not found for file %s", vaultFileName)
}
return vcommon.DecryptVaultFromBackup(encryptionSecret, vaultContent)
}🤖 Prompt for AI Agents
In common/vault.go around lines 12 to 24, the error messages returned when
failing to get the vault or when the vault is not found lack context and do not
include the original error details. Modify the error handling to wrap the
original error using fmt.Errorf with the %w verb and add descriptive context
messages, so the returned errors provide more information for debugging.
| recipe, err := policy.GetRecipe() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to parse ABI: %v", err) | ||
| return fmt.Errorf("failed to get recipe from policy: %v", err) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Wrap the original error for better debugging.
The error message should include the original error from GetRecipe.
recipe, err := policy.GetRecipe()
if err != nil {
- return fmt.Errorf("failed to get recipe from policy: %v", err)
+ return fmt.Errorf("failed to get recipe from policy: %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.
| recipe, err := policy.GetRecipe() | |
| if err != nil { | |
| return fmt.Errorf("failed to parse ABI: %v", err) | |
| return fmt.Errorf("failed to get recipe from policy: %v", err) | |
| } | |
| recipe, err := policy.GetRecipe() | |
| if err != nil { | |
| return fmt.Errorf("failed to get recipe from policy: %w", err) | |
| } |
🤖 Prompt for AI Agents
In plugin/payroll/policy.go around lines 23 to 26, the error returned from
GetRecipe is formatted as a new error string without wrapping the original
error. Update the return statement to wrap the original error using fmt.Errorf
with the %w verb instead of %v, so the original error is preserved and can be
unwrapped for better debugging.
| echain, err := chain.GetChain("ethereum") | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get ethereum chain: %v", err) | ||
| } | ||
|
|
||
| ethchain := echain.(*reth.Ethereum) | ||
|
|
There was a problem hiding this comment.
Add type assertion check to prevent panic.
The unchecked type assertion could cause a panic if the chain is not of the expected type.
Apply this diff to add proper type checking:
echain, err := chain.GetChain("ethereum")
if err != nil {
return nil, fmt.Errorf("failed to get ethereum chain: %v", err)
}
-ethchain := echain.(*reth.Ethereum)
+ethchain, ok := echain.(*reth.Ethereum)
+if !ok {
+ return nil, fmt.Errorf("chain is not of type *reth.Ethereum")
+}📝 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.
| echain, err := chain.GetChain("ethereum") | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to get ethereum chain: %v", err) | |
| } | |
| ethchain := echain.(*reth.Ethereum) | |
| echain, err := chain.GetChain("ethereum") | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to get ethereum chain: %v", err) | |
| } | |
| ethchain, ok := echain.(*reth.Ethereum) | |
| if !ok { | |
| return nil, fmt.Errorf("chain is not of type *reth.Ethereum") | |
| } |
🤖 Prompt for AI Agents
In plugin/payroll/transaction.go around lines 222 to 228, the code performs an
unchecked type assertion on echain to *reth.Ethereum, which can cause a panic if
the type is incorrect. Modify the code to use the two-value form of type
assertion to check if echain is indeed of type *reth.Ethereum. If the assertion
fails, return an appropriate error instead of proceeding, preventing a runtime
panic.
| func RuleToRecipientAndAmount(rule *rtypes.Rule) (string, string, error) { | ||
| var recipient string | ||
| var amountStr string | ||
|
|
||
| if len(rule.ParameterConstraints) == 0 { | ||
| return "", "", fmt.Errorf("no parameter constraints found") | ||
| } | ||
|
|
||
| if len(rule.ParameterConstraints) > 2 { | ||
| return "", "", fmt.Errorf("too many parameter constraints found") | ||
| } | ||
|
|
||
| for _, constraint := range rule.ParameterConstraints { | ||
| if constraint.ParameterName == "recipient" { | ||
| if constraint.Constraint.Type != rtypes.ConstraintType_CONSTRAINT_TYPE_FIXED { | ||
| return "", "", fmt.Errorf("recipient constraint is not a fixed value") | ||
| } | ||
|
|
||
| fixedValue := constraint.Constraint.GetValue().(*rtypes.Constraint_FixedValue) | ||
| recipient = fixedValue.FixedValue | ||
| } | ||
|
|
||
| if constraint.ParameterName == "amount" { | ||
| if constraint.Constraint.Type != rtypes.ConstraintType_CONSTRAINT_TYPE_FIXED { | ||
| return "", "", fmt.Errorf("amount constraint is not a fixed value") | ||
| } | ||
|
|
||
| fixedValue := constraint.Constraint.GetValue().(*rtypes.Constraint_FixedValue) | ||
| amountStr = fixedValue.FixedValue | ||
| } | ||
| } | ||
|
|
||
| return recipient, amountStr, nil | ||
| } |
There was a problem hiding this comment.
Add safe type assertions to prevent runtime panics.
The function has multiple unchecked type assertions that could cause panics at runtime.
Apply this diff to add proper type checking:
func RuleToRecipientAndAmount(rule *rtypes.Rule) (string, string, error) {
var recipient string
var amountStr string
if len(rule.ParameterConstraints) == 0 {
return "", "", fmt.Errorf("no parameter constraints found")
}
if len(rule.ParameterConstraints) > 2 {
return "", "", fmt.Errorf("too many parameter constraints found")
}
for _, constraint := range rule.ParameterConstraints {
if constraint.ParameterName == "recipient" {
if constraint.Constraint.Type != rtypes.ConstraintType_CONSTRAINT_TYPE_FIXED {
return "", "", fmt.Errorf("recipient constraint is not a fixed value")
}
- fixedValue := constraint.Constraint.GetValue().(*rtypes.Constraint_FixedValue)
+ fixedValue, ok := constraint.Constraint.GetValue().(*rtypes.Constraint_FixedValue)
+ if !ok {
+ return "", "", fmt.Errorf("failed to get fixed value for recipient")
+ }
recipient = fixedValue.FixedValue
}
if constraint.ParameterName == "amount" {
if constraint.Constraint.Type != rtypes.ConstraintType_CONSTRAINT_TYPE_FIXED {
return "", "", fmt.Errorf("amount constraint is not a fixed value")
}
- fixedValue := constraint.Constraint.GetValue().(*rtypes.Constraint_FixedValue)
+ fixedValue, ok := constraint.Constraint.GetValue().(*rtypes.Constraint_FixedValue)
+ if !ok {
+ return "", "", fmt.Errorf("failed to get fixed value for amount")
+ }
amountStr = fixedValue.FixedValue
}
}
return recipient, amountStr, nil
}📝 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.
| func RuleToRecipientAndAmount(rule *rtypes.Rule) (string, string, error) { | |
| var recipient string | |
| var amountStr string | |
| if len(rule.ParameterConstraints) == 0 { | |
| return "", "", fmt.Errorf("no parameter constraints found") | |
| } | |
| if len(rule.ParameterConstraints) > 2 { | |
| return "", "", fmt.Errorf("too many parameter constraints found") | |
| } | |
| for _, constraint := range rule.ParameterConstraints { | |
| if constraint.ParameterName == "recipient" { | |
| if constraint.Constraint.Type != rtypes.ConstraintType_CONSTRAINT_TYPE_FIXED { | |
| return "", "", fmt.Errorf("recipient constraint is not a fixed value") | |
| } | |
| fixedValue := constraint.Constraint.GetValue().(*rtypes.Constraint_FixedValue) | |
| recipient = fixedValue.FixedValue | |
| } | |
| if constraint.ParameterName == "amount" { | |
| if constraint.Constraint.Type != rtypes.ConstraintType_CONSTRAINT_TYPE_FIXED { | |
| return "", "", fmt.Errorf("amount constraint is not a fixed value") | |
| } | |
| fixedValue := constraint.Constraint.GetValue().(*rtypes.Constraint_FixedValue) | |
| amountStr = fixedValue.FixedValue | |
| } | |
| } | |
| return recipient, amountStr, nil | |
| } | |
| func RuleToRecipientAndAmount(rule *rtypes.Rule) (string, string, error) { | |
| var recipient string | |
| var amountStr string | |
| if len(rule.ParameterConstraints) == 0 { | |
| return "", "", fmt.Errorf("no parameter constraints found") | |
| } | |
| if len(rule.ParameterConstraints) > 2 { | |
| return "", "", fmt.Errorf("too many parameter constraints found") | |
| } | |
| for _, constraint := range rule.ParameterConstraints { | |
| if constraint.ParameterName == "recipient" { | |
| if constraint.Constraint.Type != rtypes.ConstraintType_CONSTRAINT_TYPE_FIXED { | |
| return "", "", fmt.Errorf("recipient constraint is not a fixed value") | |
| } | |
| fixedValue, ok := constraint.Constraint.GetValue().(*rtypes.Constraint_FixedValue) | |
| if !ok { | |
| return "", "", fmt.Errorf("failed to get fixed value for recipient") | |
| } | |
| recipient = fixedValue.FixedValue | |
| } | |
| if constraint.ParameterName == "amount" { | |
| if constraint.Constraint.Type != rtypes.ConstraintType_CONSTRAINT_TYPE_FIXED { | |
| return "", "", fmt.Errorf("amount constraint is not a fixed value") | |
| } | |
| fixedValue, ok := constraint.Constraint.GetValue().(*rtypes.Constraint_FixedValue) | |
| if !ok { | |
| return "", "", fmt.Errorf("failed to get fixed value for amount") | |
| } | |
| amountStr = fixedValue.FixedValue | |
| } | |
| } | |
| return recipient, amountStr, nil | |
| } |
🤖 Prompt for AI Agents
In plugin/payroll/transaction.go lines 672 to 705, the function uses unchecked
type assertions on constraint.Constraint.GetValue() which can cause runtime
panics if the assertion fails. To fix this, replace the direct type assertions
with safe type assertions using the "comma ok" idiom to check if the assertion
succeeded. If the assertion fails, return an appropriate error instead of
proceeding. This ensures the function handles unexpected types gracefully
without panicking.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
plugin/payroll/transaction.go (2)
222-228: The unchecked type assertion issue remains unaddressed.This is the same critical issue flagged in the past review comment. The code still performs an unchecked type assertion that could cause a runtime panic.
Apply this diff to add proper type checking:
echain, err := chain.GetChain("ethereum") if err != nil { return nil, fmt.Errorf("failed to get ethereum chain: %v", err) } -ethchain := echain.(*reth.Ethereum) +ethchain, ok := echain.(*reth.Ethereum) +if !ok { + return nil, fmt.Errorf("chain is not of type *reth.Ethereum") +}
669-702: The unchecked type assertion issue remains unaddressed.This is the same critical issue flagged in the past review comment. The function still uses unchecked type assertions on lines 687 and 696 that could cause runtime panics.
Apply this diff to add proper type checking:
func RuleToRecipientAndAmount(rule *rtypes.Rule) (string, string, error) { var recipient string var amountStr string if len(rule.ParameterConstraints) == 0 { return "", "", fmt.Errorf("no parameter constraints found") } if len(rule.ParameterConstraints) > 2 { return "", "", fmt.Errorf("too many parameter constraints found") } for _, constraint := range rule.ParameterConstraints { if constraint.ParameterName == "recipient" { if constraint.Constraint.Type != rtypes.ConstraintType_CONSTRAINT_TYPE_FIXED { return "", "", fmt.Errorf("recipient constraint is not a fixed value") } - fixedValue := constraint.Constraint.GetValue().(*rtypes.Constraint_FixedValue) + fixedValue, ok := constraint.Constraint.GetValue().(*rtypes.Constraint_FixedValue) + if !ok { + return "", "", fmt.Errorf("failed to get fixed value for recipient") + } recipient = fixedValue.FixedValue } if constraint.ParameterName == "amount" { if constraint.Constraint.Type != rtypes.ConstraintType_CONSTRAINT_TYPE_FIXED { return "", "", fmt.Errorf("amount constraint is not a fixed value") } - fixedValue := constraint.Constraint.GetValue().(*rtypes.Constraint_FixedValue) + fixedValue, ok := constraint.Constraint.GetValue().(*rtypes.Constraint_FixedValue) + if !ok { + return "", "", fmt.Errorf("failed to get fixed value for amount") + } amountStr = fixedValue.FixedValue } } return recipient, amountStr, nil }
🧹 Nitpick comments (2)
plugin/payroll/transaction.go (2)
239-243: Consider aggregating resource parsing errors instead of failing immediately.Similar to the past review comment on line 239, failing the entire transaction proposal on a single resource parsing error might not be ideal for robustness. Consider whether you want to aggregate errors or skip invalid rules.
677-679: Validate parameter constraint count more precisely.The current validation allows 0-2 parameter constraints, but both recipient and amount are required according to the recipe specification. Consider enforcing exactly 2 constraints.
- if len(rule.ParameterConstraints) > 2 { - return "", "", fmt.Errorf("too many parameter constraints found") - } + if len(rule.ParameterConstraints) != 2 { + return "", "", fmt.Errorf("exactly 2 parameter constraints required (recipient and amount), found %d", len(rule.ParameterConstraints)) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugin/payroll/transaction.go(7 hunks)
🔇 Additional comments (9)
plugin/payroll/transaction.go (9)
16-16: LGTM! Import changes support the recipe-driven refactor.The new imports for
common,chain, andrutilpackages align with the transition to recipe-based transaction proposals.Also applies to: 19-19, 21-21
207-210: LGTM! Good integration with vault storage.The code correctly integrates with the vault storage system using the new
common.GetVaultFromPolicyfunction.
212-215: LGTM! Proper address derivation from vault.The Ethereum address derivation from the vault's public key and chain code is implemented correctly.
217-220: LGTM! Recipe extraction from policy.The recipe retrieval from the plugin policy is straightforward and includes proper error handling.
245-248: LGTM! Token retrieval from chain.The token lookup by protocol ID is correctly implemented with proper error handling.
250-253: LGTM! Rule processing integration.The integration with the new
RuleToRecipientAndAmounthelper function is clean and includes proper error handling.
259-261: LGTM! Schedule time handling.The schedule time conversion using
AsTime()and interval casting is correctly implemented.
264-266: LGTM! Transition from public keys to addresses.The changes from using public keys to direct addresses (recipient, token.Address) in the transaction proposal flow are consistent and correctly implemented throughout.
Also applies to: 272-276, 284-287, 298-301
434-434: LGTM! Function signature updates for address-based approach.The function signature changes in
genUnsignedTxandevmMakeUnsignedTransferto usesenderAddressinstead of public keys are consistent with the new approach.Also applies to: 441-441, 608-608, 639-639, 643-643
| for _, constraint := range rule.ParameterConstraints { | ||
| if constraint.ParameterName == "recipient" { | ||
| if constraint.Constraint.Type != rtypes.ConstraintType_CONSTRAINT_TYPE_FIXED { | ||
| return "", "", fmt.Errorf("recipient constraint is not a fixed value") | ||
| } | ||
|
|
||
| fixedValue := constraint.Constraint.GetValue().(*rtypes.Constraint_FixedValue) | ||
| recipient = fixedValue.FixedValue | ||
| } | ||
|
|
||
| if constraint.ParameterName == "amount" { | ||
| if constraint.Constraint.Type != rtypes.ConstraintType_CONSTRAINT_TYPE_FIXED { | ||
| return "", "", fmt.Errorf("amount constraint is not a fixed value") | ||
| } | ||
|
|
||
| fixedValue := constraint.Constraint.GetValue().(*rtypes.Constraint_FixedValue) | ||
| amountStr = fixedValue.FixedValue | ||
| } | ||
| } | ||
|
|
||
| return recipient, amountStr, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for missing required parameters.
The function should validate that both recipient and amount were found in the constraints to ensure completeness.
for _, constraint := range rule.ParameterConstraints {
// ... existing constraint processing ...
}
+ if recipient == "" {
+ return "", "", fmt.Errorf("recipient parameter constraint not found")
+ }
+ if amountStr == "" {
+ return "", "", fmt.Errorf("amount parameter constraint not found")
+ }
+
return recipient, amountStr, nil📝 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.
| for _, constraint := range rule.ParameterConstraints { | |
| if constraint.ParameterName == "recipient" { | |
| if constraint.Constraint.Type != rtypes.ConstraintType_CONSTRAINT_TYPE_FIXED { | |
| return "", "", fmt.Errorf("recipient constraint is not a fixed value") | |
| } | |
| fixedValue := constraint.Constraint.GetValue().(*rtypes.Constraint_FixedValue) | |
| recipient = fixedValue.FixedValue | |
| } | |
| if constraint.ParameterName == "amount" { | |
| if constraint.Constraint.Type != rtypes.ConstraintType_CONSTRAINT_TYPE_FIXED { | |
| return "", "", fmt.Errorf("amount constraint is not a fixed value") | |
| } | |
| fixedValue := constraint.Constraint.GetValue().(*rtypes.Constraint_FixedValue) | |
| amountStr = fixedValue.FixedValue | |
| } | |
| } | |
| return recipient, amountStr, nil | |
| } | |
| for _, constraint := range rule.ParameterConstraints { | |
| if constraint.ParameterName == "recipient" { | |
| if constraint.Constraint.Type != rtypes.ConstraintType_CONSTRAINT_TYPE_FIXED { | |
| return "", "", fmt.Errorf("recipient constraint is not a fixed value") | |
| } | |
| fixedValue := constraint.Constraint.GetValue().(*rtypes.Constraint_FixedValue) | |
| recipient = fixedValue.FixedValue | |
| } | |
| if constraint.ParameterName == "amount" { | |
| if constraint.Constraint.Type != rtypes.ConstraintType_CONSTRAINT_TYPE_FIXED { | |
| return "", "", fmt.Errorf("amount constraint is not a fixed value") | |
| } | |
| fixedValue := constraint.Constraint.GetValue().(*rtypes.Constraint_FixedValue) | |
| amountStr = fixedValue.FixedValue | |
| } | |
| } | |
| if recipient == "" { | |
| return "", "", fmt.Errorf("recipient parameter constraint not found") | |
| } | |
| if amountStr == "" { | |
| return "", "", fmt.Errorf("amount parameter constraint not found") | |
| } | |
| return recipient, amountStr, nil | |
| } |
🤖 Prompt for AI Agents
In plugin/payroll/transaction.go around lines 681 to 702, the code extracts
recipient and amount from constraints but does not validate if both were found.
Add checks after the loop to verify that recipient and amountStr are not empty
or unset, and if either is missing, return an error indicating the missing
required parameter to ensure completeness of the data.
|
@RaghavSood Thanks for PR, looks great, please review my commit, I think that change fixes recipe rules list 1150df3
|
This PR gets us to an MVP stage for recipe-based payroll transaction proposals.
It removes the legacy
PayrollPolicyand transitions to solely using recipes for transaction construction and validationIt still has a few rough edges, and is missing many edge cases, but it is able to derive the recipient and amounts from the recipe, and use them to propose a valid transaction based on the vault address.
Summary by CodeRabbit