Conversation
WalkthroughImports and type references were standardized across modules; API GetVault response now uses types.VaultGetResponse. Payroll signing flow was refactored to construct EVM sign requests via a factory and adjusted Chain handling. Fees plugin integrated the EVM SDK and rtypes. Transaction payload encoding switched to base64 in fees transactions. Minor import cleanups and dependency version updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant PayrollPlugin as Payroll Plugin
participant SignReqFactory as NewPluginKeysignRequestEvm
participant Queue as Sign Queue
User->>PayrollPlugin: ProposeTransactions(policy, txs)
PayrollPlugin->>SignReqFactory: Build sign request (policy, txID, chain, unsignedTx)
alt success
SignReqFactory-->>PayrollPlugin: *PluginKeysignRequestEvm
PayrollPlugin->>Queue: Append sign request
Queue-->>User: Enqueued
else error
SignReqFactory-->>PayrollPlugin: error
PayrollPlugin-->>User: return error
end
sequenceDiagram
autonumber
actor Caller
participant FeesPlugin as Fees Plugin
participant EvmSDK as evm.SDK
participant Engine as Policy Engine
Caller->>FeesPlugin: Suggest(...)
FeesPlugin->>EvmSDK: Initialize/use SDK
FeesPlugin->>Engine: Evaluate(chain, policy, context)
Engine-->>FeesPlugin: *rtypes.PolicySuggest
FeesPlugin-->>Caller: *rtypes.PolicySuggest
sequenceDiagram
autonumber
actor Client
participant FeesTx as Fees Transaction
note over FeesTx: Change: hex -> base64
Client->>FeesTx: proposeTransactions(unsignedTxBytes)
FeesTx->>FeesTx: Base64-encode tx bytes
FeesTx-->>Client: Payload with base64 Transaction
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (10)
💤 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)
🧰 Additional context used🧠 Learnings (5)📓 Common learnings📚 Learning: 2025-07-24T19:34:47.441ZApplied to files:
📚 Learning: 2025-06-18T18:23:20.077ZApplied to files:
📚 Learning: 2025-06-18T18:22:06.358ZApplied to files:
📚 Learning: 2025-06-18T18:28:19.759ZApplied to files:
🔇 Additional comments (4)
✨ 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. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugin/fees/transaction.go (1)
98-101: Token address check always fails;tokenis never set.
tokenis declared but never assigned from the policy constraints, so the comparison againstusdc.Addresswill fail (empty vs USDC) and abort the flow.Minimal safe fix to avoid false negatives until token parsing is implemented:
- if gcommon.HexToAddress(token) != gcommon.HexToAddress(usdc.Address) { + if token != "" && gcommon.HexToAddress(token) != gcommon.HexToAddress(usdc.Address) { return nil, fmt.Errorf("token address does not match usdc address") }If the intent is to enforce USDC strictly, extract the token address from the rule constraints (e.g., a parameter like "token" or "contract"). I can help wire that in if you share the expected constraint name.
🧹 Nitpick comments (6)
scripts/dev/add_balance/main.go (1)
61-66: Address derivation update is correct; consider two small improvements.
- The call to
address.GetAddress(..., vgcommon.Ethereum)matches the new API and looks good.- Optional fixes unrelated to the refactor:
- The script builds
stateDirbut doesn't use it when reading the public key; it reads fromvaults/...instead ofstateDir. Consider usingfilepath.Join(stateDir, vaultName, "public_key").- Gas estimation using an empty
ethereum.CallMsg{}can be misleading. For simple ETH transfer, estimate withToandValueset.If you want, I can draft a small patch to apply these tweaks.
plugin/fees/transaction.go (1)
121-133: Double-check hash semantics for EVM signing.You send:
- Message: keccak256(tx) bytes (base64-encoded)
- Hash: sha256(Message)
If TSS signs the provided Hash using HashFunction=SHA256, the on-chain signature must still correspond to the keccak256(tx) preimage. Ensure the signer/TSS layer is not double-hashing in a way that breaks EVM validity. If the TSS expects the final digest to sign, consider setting
Messageto raw tx preimage orHashFunctionto "NONE" withHashequal totxHashToSign.Bytes().If you want, I can cross-check this against the TSS signer's expectations and suggest the precise struct values.
plugin/payroll/transaction.go (3)
175-180: Potential type mismatch for ChainID in tx indexer DTO.storage.CreateTxDto.ChainID may expect a string or a different chain type than vgcommon.Chain. To avoid tight coupling and potential compile-time mismatch, consider passing the string representation.
If ChainID ultimately persists as a string in the indexer, prefer:
- ChainID: chain, + ChainID: chain.String(),
185-189: Typo in error message ("unsifned").Minor nit in the error text returned on sign-request creation failure.
Apply this diff:
- if e != nil { - return fmt.Errorf("failed to create unsifned request: %w", e) - } + if e != nil { + return fmt.Errorf("failed to create unsigned request: %w", e) + }
229-230: Avoid hardcoding chain in logs; use the request’s chain for future-proofing.Using a constant makes logs misleading if/when more chains are supported.
Apply this diff:
- "chain": vgcommon.Ethereum.String(), + "chain": signRequest.Chain,api/plugin.go (1)
261-265: Nit: Fix error message punctuation.Minor readability improvement in the error message.
Apply this diff:
- return nil, fmt.Errorf("failed to decrypt vault,err: %w", err) + return nil, fmt.Errorf("failed to decrypt vault, err: %w", err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
api/plugin.go(3 hunks)api/server.go(6 hunks)common/vault.go(2 hunks)go.mod(2 hunks)plugin/fees/transaction.go(5 hunks)plugin/payroll/payroll.go(2 hunks)plugin/payroll/policy.go(1 hunks)plugin/payroll/transaction.go(7 hunks)scripts/dev/add_balance/main.go(2 hunks)
🧰 Additional context used
🧠 Learnings (14)
📓 Common learnings
Learnt from: neavra
PR: vultisig/plugin#157
File: api/plugin.go:236-236
Timestamp: 2025-08-20T13:51:05.194Z
Learning: In the vultisig/plugin codebase migration to vultisig-go, SystemMigrations() and vultisig_validator functionality were not moved from the verifier package to vultisig-go. This means some imports of github.com/vultisig/verifier/common are intentionally retained for accessing these specific functions, while vault-related types and common functionality were migrated to vultisig-go.
Learnt from: neavra
PR: vultisig/plugin#157
File: api/server.go:22-24
Timestamp: 2025-08-20T13:51:41.769Z
Learning: In the vultisig-go migration, only vault-related functions (GetVaultBackupFilename, DecryptVaultFromBackup) were moved from github.com/vultisig/verifier/common to github.com/vultisig/vultisig-go/common. Other functions like SystemMigrations() remained in the verifier/common package, so files using these non-migrated functions should keep the original verifier/common import.
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#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: neavra
PR: vultisig/plugin#157
File: api/server.go:22-24
Timestamp: 2025-08-20T13:51:41.769Z
Learning: SystemMigrations() function was not moved from github.com/vultisig/verifier/common to github.com/vultisig/vultisig-go/common during the vultisig-go migration, so files that use SystemMigrations() should keep the verifier/common import.
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.
📚 Learning: 2025-07-24T19:34:47.441Z
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.
Applied to files:
plugin/payroll/payroll.goplugin/payroll/policy.goapi/plugin.goplugin/payroll/transaction.goplugin/fees/transaction.go
📚 Learning: 2025-08-20T13:51:05.194Z
Learnt from: neavra
PR: vultisig/plugin#157
File: api/plugin.go:236-236
Timestamp: 2025-08-20T13:51:05.194Z
Learning: In the vultisig/plugin codebase migration to vultisig-go, SystemMigrations() and vultisig_validator functionality were not moved from the verifier package to vultisig-go. This means some imports of github.com/vultisig/verifier/common are intentionally retained for accessing these specific functions, while vault-related types and common functionality were migrated to vultisig-go.
Applied to files:
plugin/payroll/payroll.goscripts/dev/add_balance/main.goplugin/payroll/policy.goapi/plugin.gogo.modapi/server.goplugin/fees/transaction.gocommon/vault.go
📚 Learning: 2025-05-07T08:23:45.882Z
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.
Applied to files:
plugin/payroll/payroll.goscripts/dev/add_balance/main.goapi/plugin.gogo.modapi/server.goplugin/fees/transaction.go
📚 Learning: 2025-08-20T13:51:41.769Z
Learnt from: neavra
PR: vultisig/plugin#157
File: api/server.go:22-24
Timestamp: 2025-08-20T13:51:41.769Z
Learning: In the vultisig-go migration, only vault-related functions (GetVaultBackupFilename, DecryptVaultFromBackup) were moved from github.com/vultisig/verifier/common to github.com/vultisig/vultisig-go/common. Other functions like SystemMigrations() remained in the verifier/common package, so files using these non-migrated functions should keep the original verifier/common import.
Applied to files:
plugin/payroll/payroll.goscripts/dev/add_balance/main.goapi/plugin.gogo.modapi/server.goplugin/fees/transaction.gocommon/vault.go
📚 Learning: 2025-08-20T13:51:41.769Z
Learnt from: neavra
PR: vultisig/plugin#157
File: api/server.go:22-24
Timestamp: 2025-08-20T13:51:41.769Z
Learning: SystemMigrations() function was not moved from github.com/vultisig/verifier/common to github.com/vultisig/vultisig-go/common during the vultisig-go migration, so files that use SystemMigrations() should keep the verifier/common import.
Applied to files:
plugin/payroll/payroll.goscripts/dev/add_balance/main.goapi/plugin.gogo.modapi/server.gocommon/vault.go
📚 Learning: 2025-06-18T18:20:59.510Z
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.
Applied to files:
plugin/payroll/payroll.go
📚 Learning: 2025-06-18T18:22:06.358Z
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.
Applied to files:
plugin/payroll/payroll.goplugin/payroll/transaction.go
📚 Learning: 2025-07-09T22:23:17.348Z
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.
Applied to files:
plugin/payroll/policy.go
📚 Learning: 2025-07-02T04:55:36.331Z
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.
Applied to files:
go.mod
📚 Learning: 2025-06-18T18:23:20.077Z
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.
Applied to files:
plugin/payroll/transaction.goplugin/fees/transaction.go
📚 Learning: 2025-06-18T18:28:19.759Z
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.
Applied to files:
plugin/payroll/transaction.go
📚 Learning: 2025-07-31T15:43:42.631Z
Learnt from: garry-sharp
PR: vultisig/plugin#146
File: plugin/fees/transaction.go:158-160
Timestamp: 2025-07-31T15:43:42.631Z
Learning: In the vultisig/plugin fees plugin, the public ProposeTransactions method intentionally returns "not implemented" because the fees plugin v2 uses a run-based architecture instead of the standard plugin template pattern. The actual transaction proposal logic is implemented in the private proposeTransactions method that accepts a types.FeeRun parameter, allowing fees to be batched and tracked through fee runs rather than individual policy-based transactions.
Applied to files:
plugin/fees/transaction.go
📚 Learning: 2025-07-02T04:58:30.139Z
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.
Applied to files:
common/vault.go
🧬 Code Graph Analysis (3)
api/plugin.go (1)
scripts/dev/create_fee_policy/main.go (1)
DecryptVaultFromBackup(205-234)
api/server.go (1)
scripts/dev/create_fee_policy/main.go (1)
DecryptVaultFromBackup(205-234)
common/vault.go (1)
scripts/dev/create_fee_policy/main.go (1)
DecryptVaultFromBackup(205-234)
🔇 Additional comments (30)
plugin/payroll/payroll.go (2)
16-16: Import migration to vgcommon looks correct.Switching to
github.com/vultisig/vultisig-go/commonis consistent with the repo-wide refactor. No issues spotted with the alias or usage.
50-57: Correct use of vgcommon for Ethereum chain ID.
vgcommon.Ethereum.EvmID()usage is appropriate and error-handled. The resultingethSDK init remains unchanged and correct.plugin/payroll/policy.go (1)
32-35: Engine.Evaluate chain parameter change is aligned with refactor.Passing
keysignMessage.Chaindirectly removes the rcommon wrapper and matches the new abstractions. The base64 decode ofMessageis consistent with the fees plugin changes.scripts/dev/add_balance/main.go (1)
18-20: Imports updated to vultisig-go packages — good.Using
vultisig-go/addressandvgcommonaligns with the migration.go.mod (2)
21-25: Dependency bumps and addition of vultisig-go align with code changes.The versions for recipes, verifier, and the addition of
vultisig-goare consistent with the refactor. Nothing stands out as problematic here.
3-3: Sanity Check Passed: go.mod/go.sum are tidy and module builds cleanly• go env GOVERSION reports go1.24.2
•go mod tidycompleted without errors and updated dependencies
•go list -m allsucceeded with no missing or conflicting modules
•rg 'vultisig-go'confirms imports ofgithub.com/vultisig/vultisig-go(e.g. in plugin/payroll, plugin/fees, api/server.go, api/plugin.go, common/vault.go)No further action required.
plugin/fees/transaction.go (4)
12-15: Imports align with new chain/address abstractions.Adding
vgcommon,address, and EVM helpers (hexutil,etypes) matches the rest of the refactor. Resolver import looks fine.Also applies to: 20-27
50-55: Address derivation correctly migrated to vultisig-go.
address.GetAddress(..., vgcommon.Ethereum)is consistent with the migration.
247-251: Engine.Evaluate now uses chain from the message — consistent with policy refactor.Decoding base64 and evaluating with
keysignMessage.Chainmatches the new pattern used in payroll.
128-131: Changed encodings: Message/Transaction are now base64 — update downstream decode logic accordingly.You switched:
Messages[0].Messageto base64 of the tx hash-to-sign bytes.Transactionto base64 of the raw unsigned tx bytes.However,
initSignstill decodesreq.Transactionusing a hex decoder, which will now fail. UpdateinitSignto base64-decodereq.Transactionbefore sending.Apply this diff within
initSign:- txBytes, txErr := hexutilDecode(req.Transaction) + txBytes, txErr := base64.StdEncoding.DecodeString(req.Transaction) r, rErr := hexutilDecode(sig.R) s, sErr := hexutilDecode(sig.S) v, vErr := hexutilDecode(sig.RecoveryID)And update the decode for logging to use the already-decoded bytes:
- erc20tx, err := decodeTx(req.Transaction) + // If decodeTx expects hex, convert the decoded bytes to hex + erc20tx, err := decodeTx(hexutil.Encode(txBytes))Alternatively, if you control
decodeTx, add adecodeTxBytes([]byte)variant and call that instead.Also applies to: 140-141
⛔ Skipped due to learnings
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.plugin/payroll/transaction.go (5)
22-24: Imports migration to vultisig-go and internal scheduler look correct.Switching to github.com/vultisig/vultisig-go/{address,common} and using the local plugin/internal/scheduler aligns with the refactor and reduces verifier/* coupling.
Also applies to: 27-27
114-118: Confirm GetAddress semantics and returned format.Using address.GetAddress(vault.PublicKeyEcdsa, vault.HexChainCode, vgcommon.Ethereum) is the right direction. Please double-check:
- The function’s chain argument matches the expected type (vgcommon.Chain).
- ethAddress is in a format compatible with ecommon.HexToAddress (with or without 0x should be fine, but consistency helps).
124-128: LGTM: Centralized chain selection.Defining chain := vgcommon.Ethereum simplifies downstream branching and keeps the chain identity cohesive.
191-193: LGTM: Appending the constructed sign request.Appending the dereferenced signRequest within a mutex protects the slice and is concurrency-safe.
312-317: LGTM: Chain type migration and switch-case.Switching genUnsignedTx’s chain param to vgcommon.Chain is consistent with the file-level migration. The case vgcommon.Ethereum path is correct.
api/plugin.go (3)
19-20: LGTM: Migrated vault helpers to vgcommon.Importing github.com/vultisig/vultisig-go/common aligns with the vultisig-go migration for vault utilities.
236-236: Verify derive-path API parity in vgcommon.Confirm vgcommon.Ethereum.GetDerivePath() returns the same path as the previous verifier/common implementation to avoid signature verification regressions.
254-254: LGTM: Using vgcommon.GetVaultBackupFilename.Vault filename derivation now consistently comes from vultisig-go.
common/vault.go (4)
8-10: LGTM: Imports align with new ownership split.Keeping vault.Storage under verifier/vault and moving vault helpers to vultisig-go/common matches the migration notes.
13-14: LGTM: Switch to vgcommon for file naming.Using vgcommon.GetVaultBackupFilename(policy.PublicKey, policy.PluginID.String()) is consistent across the PR.
13-14: AI summary inconsistency: the nil-check remains.The AI summary states the explicit nil-check for vaultContent was removed, but it’s still present (Lines 19-21). Code is fine; just noting the discrepancy for tracking.
Also applies to: 23-24
23-24: LGTM: Decrypt via vgcommon.Returning vgcommon.DecryptVaultFromBackup(encryptionSecret, vaultContent) keeps the function’s signature intact while sourcing from vultisig-go.
api/server.go (8)
22-24: LGTM: Vault-related imports moved to vultisig-go.Imports for vgcommon and vultisig-go/types are consistent with the migration and usage in this file.
163-169: LGTM: Unified vault filename derivation.Using vgcommon.GetVaultBackupFilename here mirrors other call sites and reduces drift.
171-175: LGTM: Decrypt via vgcommon.vgcommon.DecryptVaultFromBackup maintains consistency across server endpoints.
177-184: Confirm response type change won’t break clients.Switching to types.VaultGetResponse (from vultisig-go/types) should keep the JSON shape, but it’s a public API. Validate that field names and formats are identical for existing consumers.
208-215: LGTM: Vault filename derivation in SignMessages.Consistent use of vgcommon.GetVaultBackupFilename for signing flow.
217-220: LGTM: Decrypt to validate before enqueuing keysign.Decrypting the vault backup to validate and short-circuit failures is appropriate.
270-274: LGTM: DeleteVault uses vgcommon for path derivation.Consistent with other endpoints.
298-304: LGTM: ExistVault uses vgcommon for path derivation.Consistent with GetVault and SignMessages endpoints.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
plugin/fees/fees.go (1)
325-333: Improve vault “not found” error context in plugin/feesvgcommon.GetVaultBackupFilename semantics look correct (no lingering calls to the old verifier/common helper were found). However, when
vaultContent == nilthe generic"vault not found"error can make it hard to trace filename mismatches at runtime. Please include the public key and expected filename for easier debugging:- if vaultContent == nil { - return fmt.Errorf("vault not found") - } + if vaultContent == nil { + return fmt.Errorf( + "vault not found for publicKey=%s, expected filename=%s", + feePolicy.PublicKey, + vaultFileName, + ) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
plugin/fees/fees.go(2 hunks)scripts/dev/create_fee_policy/main.go(2 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: neavra
PR: vultisig/plugin#157
File: api/plugin.go:236-236
Timestamp: 2025-08-20T13:51:05.194Z
Learning: In the vultisig/plugin codebase migration to vultisig-go, SystemMigrations() and vultisig_validator functionality were not moved from the verifier package to vultisig-go. This means some imports of github.com/vultisig/verifier/common are intentionally retained for accessing these specific functions, while vault-related types and common functionality were migrated to vultisig-go.
Learnt from: neavra
PR: vultisig/plugin#157
File: api/server.go:22-24
Timestamp: 2025-08-20T13:51:41.769Z
Learning: In the vultisig-go migration, only vault-related functions (GetVaultBackupFilename, DecryptVaultFromBackup) were moved from github.com/vultisig/verifier/common to github.com/vultisig/vultisig-go/common. Other functions like SystemMigrations() remained in the verifier/common package, so files using these non-migrated functions should keep the original verifier/common import.
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: 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: neavra
PR: vultisig/plugin#157
File: api/server.go:22-24
Timestamp: 2025-08-20T13:51:41.769Z
Learning: SystemMigrations() function was not moved from github.com/vultisig/verifier/common to github.com/vultisig/vultisig-go/common during the vultisig-go migration, so files that use SystemMigrations() should keep the verifier/common import.
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.
📚 Learning: 2025-08-20T13:51:41.769Z
Learnt from: neavra
PR: vultisig/plugin#157
File: api/server.go:22-24
Timestamp: 2025-08-20T13:51:41.769Z
Learning: In the vultisig-go migration, only vault-related functions (GetVaultBackupFilename, DecryptVaultFromBackup) were moved from github.com/vultisig/verifier/common to github.com/vultisig/vultisig-go/common. Other functions like SystemMigrations() remained in the verifier/common package, so files using these non-migrated functions should keep the original verifier/common import.
Applied to files:
scripts/dev/create_fee_policy/main.goplugin/fees/fees.go
📚 Learning: 2025-08-20T13:51:05.194Z
Learnt from: neavra
PR: vultisig/plugin#157
File: api/plugin.go:236-236
Timestamp: 2025-08-20T13:51:05.194Z
Learning: In the vultisig/plugin codebase migration to vultisig-go, SystemMigrations() and vultisig_validator functionality were not moved from the verifier package to vultisig-go. This means some imports of github.com/vultisig/verifier/common are intentionally retained for accessing these specific functions, while vault-related types and common functionality were migrated to vultisig-go.
Applied to files:
scripts/dev/create_fee_policy/main.goplugin/fees/fees.go
📚 Learning: 2025-05-07T08:23:45.882Z
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.
Applied to files:
scripts/dev/create_fee_policy/main.goplugin/fees/fees.go
📚 Learning: 2025-07-10T20:41:44.025Z
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.
Applied to files:
plugin/fees/fees.go
📚 Learning: 2025-07-24T19:34:47.441Z
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.
Applied to files:
plugin/fees/fees.go
📚 Learning: 2025-08-20T13:51:41.769Z
Learnt from: neavra
PR: vultisig/plugin#157
File: api/server.go:22-24
Timestamp: 2025-08-20T13:51:41.769Z
Learning: SystemMigrations() function was not moved from github.com/vultisig/verifier/common to github.com/vultisig/vultisig-go/common during the vultisig-go migration, so files that use SystemMigrations() should keep the verifier/common import.
Applied to files:
plugin/fees/fees.go
📚 Learning: 2025-07-02T04:58:30.139Z
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.
Applied to files:
plugin/fees/fees.go
📚 Learning: 2025-07-02T04:58:30.139Z
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 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.
Applied to files:
plugin/fees/fees.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). (1)
- GitHub Check: build
🔇 Additional comments (3)
scripts/dev/create_fee_policy/main.go (2)
21-25: Import switch to vgcommon aligns with the migration planReplacing the verifier/common decryption import with vgcommon is consistent with the vultisig-go migration. Keeping vtypes from verifier/types is also correct per prior migrations.
223-227: Vault helper migration consistency verifiedAll calls to DecryptVault and GetVaultBackupFilename now correctly reference vgcommon, and go.mod includes the vultisig-go dependency. The only remaining imports of github.com/vultisig/verifier/common are in storage/postgres/db_system.go and api/server.go for non-vault functions (e.g. SystemMigrations, vultisig_validator), per intended migration scope. No further changes required.
plugin/fees/fees.go (1)
12-20: Imports updated correctly for vultisig-go migration
- Adding vgcommon and swapping out vault helpers is correct.
- Keeping verifier/vault and verifier/plugin imports is appropriate and consistent with the split where only vault helper utilities moved to vultisig-go.
No issues from this change.
|
please rebase your pr @evgensheff |
evgensheff
force-pushed
the
fix/encoding
branch
from
bb370d0 to
8ed533c
Compare
done |
3 participants
Updates related to verifier refactor
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores
Scripts