Conversation
WalkthroughThe codebase migrates dependencies from verifier/recipes modules to vultisig-go (common, types, address), updates API/server responses to use vgtypes, and adjusts vault utilities to vgcommon with a new nil guard. Multiple plugins and dev scripts update chain/address references. go.mod adds vultisig-go and bumps versions. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant S as API Server
participant FS as Filesystem
participant VG as vgcommon (vultisig-go/common)
C->>S: GET /vault (publicKey, pluginID)
S->>FS: Build path via VG.GetVaultBackupFilename(...)
FS-->>S: Vault backup bytes (or not found)
alt Backup found
S->>VG: DecryptVaultFromBackup(bytes, password/params)
VG-->>S: Decrypted vault data
S-->>C: 200 JSON (vgtypes.VaultGetResponse)
else Not found / error
S-->>C: 4xx/5xx error
end
sequenceDiagram
autonumber
participant U as Caller
participant V as common/vault.go
participant FS as Filesystem
participant VG as vgcommon
U->>V: GetVault(...)
V->>FS: Read vault backup via VG.GetVaultBackupFilename(...)
FS-->>V: vaultContent (bytes or nil)
alt vaultContent == nil
Note over V: New guard
V-->>U: error "vault not found"
else bytes present
V->>VG: DecryptVaultFromBackup(...)
VG-->>V: Decrypted vault
V-->>U: vault
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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. ✨ 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: 3
🧹 Nitpick comments (15)
plugin/payroll/payroll.go (1)
16-16: Use a consistent alias (vgcommon) for vultisig-go/common across the repoNit: Other files use the vgcommon alias to disambiguate from go-ethereum/common. Consider aligning for consistency.
Apply this diff to standardize the alias and call site:
- "github.com/vultisig/vultisig-go/common" + vgcommon "github.com/vultisig/vultisig-go/common" @@ - ethEvmChainID, err := common.Ethereum.EvmID() + ethEvmChainID, err := vgcommon.Ethereum.EvmID()go.mod (1)
21-25: Dependency updates align with the migration — ensure tidy and CI build passrecipes/verifier bumps and the new vultisig-go requirement look aligned with code changes.
- Run go mod tidy and ensure CI can resolve the new pseudo-version of vultisig-go.
- When cutting a release, consider pinning vultisig-go to a tag if available (team is fine with master during dev per prior practice).
common/vault.go (1)
13-23: Propagate underlying errors and include filename context; also wrap decryption error.Minor ergonomics: preserve error context and include the filename to aid debugging. Also wrap the decryption error with context.
Apply:
- vaultContent, err := s.GetVault(vaultFileName) + vaultContent, err := s.GetVault(vaultFileName) if err != nil { - return nil, fmt.Errorf("failed to get vault") + return nil, fmt.Errorf("failed to get vault file %q: %w", vaultFileName, err) } if vaultContent == nil { - return nil, fmt.Errorf("vault not found") + return nil, fmt.Errorf("vault not found: %s", vaultFileName) } - return vgcommon.DecryptVaultFromBackup(encryptionSecret, vaultContent) + v, err := vgcommon.DecryptVaultFromBackup(encryptionSecret, vaultContent) + if err != nil { + return nil, fmt.Errorf("failed to decrypt vault backup %q: %w", vaultFileName, err) + } + return v, nilscripts/dev/create_fee_policy/main.go (1)
222-223: Refactor to reuse vgcommon’s DecryptVaultFromBackupThe local
DecryptVaultFromBackupinscripts/dev/create_fee_policy/main.goduplicates the shared helper invgcommon. To avoid drift and keep decryption logic centralized:• Remove the entire local function at lines 205–… in
scripts/dev/create_fee_policy/main.go.
• Replace calls to it with:vault, err := vgcommon.DecryptVaultFromBackup(password, vaultBackupRaw) if err != nil { return nil, fmt.Errorf("failed to decrypt vault from backup: %w", err) }• If you still need the raw JSON bytes in this script, consider extending
vgcommon.DecryptVaultFromBackupto also return them; otherwise drop the extra return value here.This keeps decryption logic DRY and prevents future drift.
api/plugin.go (1)
254-263: Add a nil/empty guard for vault content and improve error messages.Mirror the nil guard you added elsewhere, and wrap errors with context (filename) for faster debugging.
- fileName := vgcommon.GetVaultBackupFilename(publicKeyECDSA, pluginId) - vaultContent, err := s.vaultStorage.GetVault(fileName) + fileName := vgcommon.GetVaultBackupFilename(publicKeyECDSA, pluginId) + vaultContent, err := s.vaultStorage.GetVault(fileName) if err != nil { - s.logger.WithError(err).Error("fail to get vault") - return nil, fmt.Errorf("failed to get vault, err: %w", err) + s.logger.WithError(err).WithField("file", fileName).Error("fail to get vault") + return nil, fmt.Errorf("failed to get vault file %q: %w", fileName, err) } - v, err := vgcommon.DecryptVaultFromBackup(s.cfg.EncryptionSecret, vaultContent) + if len(vaultContent) == 0 { + return nil, fmt.Errorf("vault not found: %s", fileName) + } + v, err := vgcommon.DecryptVaultFromBackup(s.cfg.EncryptionSecret, vaultContent) if err != nil { - return nil, fmt.Errorf("failed to decrypt vault,err: %w", err) + return nil, fmt.Errorf("failed to decrypt vault backup %q: %w", fileName, err) } return v, nilplugin/fees/transaction.go (3)
51-53: Address derivation uses vgcommon.Ethereum — consider future-proofing via config.If fees may target other EVM chains later, consider deriving with a chain variable/config instead of hardcoding vgcommon.Ethereum, similar to how you use fp.config.ChainId for signing.
-ethAddress, _, _, err := address.GetAddress(vault.PublicKeyEcdsa, vault.HexChainCode, vgcommon.Ethereum) +chain := vgcommon.Ethereum // or map from fp.config.ChainId if/when multi-chain arrives +ethAddress, _, _, err := address.GetAddress(vault.PublicKeyEcdsa, vault.HexChainCode, chain)
129-133: Consistent chain propagation into KeysignMessage.You’re setting Chain to vgcommon.Ethereum. If/when you generalize chain selection (above), propagate the same chain variable here.
- Chain: vgcommon.Ethereum, + Chain: chain,
248-250: Type cast likely unnecessary.If KeysignMessage.Chain is already vgcommon.Chain, the cast is redundant. Keep if it resolves a mismatch, otherwise you can drop it.
plugin/payroll/transaction.go (3)
118-121: Address derivation switched to vgcommon.Ethereum — consistent with the migration.Matches the broader change set. If payroll expands beyond Ethereum later, consider passing a chain variable resolved from config.
190-191: ChainID cast — likely redundant.If CreateTxDto.ChainID is already vgcommon.Chain, the explicit cast can be removed. Keep if the DTO expects a different type.
186-218: Use 0x-Prefixed Hex Encoding for Transaction for ConsistencyTo align with the fees plugin and other modules, encode the unsigned transaction with a leading “0x” prefix. Both
ecommon.FromHexand downstream systems accept the prefix and this standardizes representation across plugins.• File:
plugin/payroll/transaction.go, around line 178
• Replace:- txHex := ecommon.Bytes2Hex(tx) + // use 0x-prefixed hex for consistency with other plugins + txHex := hexutil.Encode(tx)api/server.go (4)
163-184: Ensure file-naming compatibility and response JSON parity after migrationTwo things to double-check here:
- File path generation: vgcommon.GetVaultBackupFilename must produce the exact same filename scheme as before, or existing vault backups will not be found.
- API response shape: vgtypes.VaultGetResponse should retain the same JSON field names/tags as before to avoid any breaking change for clients.
Minor nit:
- Local var naming: consider renaming pluginId → pluginID for Go style consistency (ID as an initialism).
If there’s any chance the filename scheme differs (e.g., 0x prefix handling, case changes), consider adding a small fallback to try the legacy filename before failing. At minimum, please verify with a real backup sample.
Additionally, to keep error handling consistent and sanitized, consider this tweak for the read failure path:
content, err := s.vaultStorage.GetVault(filePathName) if err != nil { wrappedErr := fmt.Errorf("fail to read file in GetVault, err: %w", err) s.logger.Error(wrappedErr) - return wrappedErr + return c.JSON(http.StatusInternalServerError, NewErrorResponse("fail to read vault")) }
208-220: SignMessages: handle missing vault as 404 and avoid duplicate logs; sanitize error responses
- If the vault file doesn’t exist, returning 404 is clearer and avoids leaking internal errors.
- Remove duplicate logging (Infof + Error). One structured error log is enough.
- Return a sanitized JSON error rather than a raw error for consistency with other handlers.
Apply this diff within the selected lines:
@@ - content, err := s.vaultStorage.GetVault(filePathName) - if err != nil { - wrappedErr := fmt.Errorf("fail to read file in SignMessages, err: %w", err) - s.logger.Infof("fail to read file in SignMessages, err: %v", err) - s.logger.Error(wrappedErr) - return wrappedErr - } + content, err := s.vaultStorage.GetVault(filePathName) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + return c.NoContent(http.StatusNotFound) + } + s.logger.WithError(err).Error("fail to read file in SignMessages") + return c.JSON(http.StatusInternalServerError, NewErrorResponse("fail to read vault")) + } @@ - _, err = vgcommon.DecryptVaultFromBackup(s.cfg.EncryptionSecret, content) - if err != nil { - return fmt.Errorf("fail to decrypt vault from the backup, err: %w", err) - } + _, err = vgcommon.DecryptVaultFromBackup(s.cfg.EncryptionSecret, content) + if err != nil { + s.logger.WithError(err).Error("fail to decrypt vault from backup") + return c.JSON(http.StatusInternalServerError, NewErrorResponse("fail to decrypt vault from backup")) + }Outside the selected lines, add the necessary imports:
import ( "errors" "os" )
270-275: DeleteVault: make delete idempotent and keep errors sanitizedTreat a missing file as success (No Content), which is a common idempotent delete behavior and avoids leaking existence. Other errors should remain 500.
- if err := s.vaultStorage.DeleteFile(fileName); err != nil { - return c.JSON(http.StatusInternalServerError, NewErrorResponse(err.Error())) - } + if err := s.vaultStorage.DeleteFile(fileName); err != nil { + if errors.Is(err, os.ErrNotExist) { + return c.NoContent(http.StatusNoContent) + } + return c.JSON(http.StatusInternalServerError, NewErrorResponse(err.Error())) + }Outside the selected lines, ensure these imports exist (if not already added by prior changes):
import ( "errors" "os" )
298-304: ExistVault: prefer 404 for not found, 500 for backend errorsReturning 400 conflates non-existence with client error. Consider:
- 404 when the vault does not exist.
- 500 when the storage check fails.
- exist, err := s.vaultStorage.Exist(filePathName) - if err != nil || !exist { - return c.NoContent(http.StatusBadRequest) - } - return c.NoContent(http.StatusOK) + exist, err := s.vaultStorage.Exist(filePathName) + if err != nil { + return c.JSON(http.StatusInternalServerError, NewErrorResponse(err.Error())) + } + if !exist { + return c.NoContent(http.StatusNotFound) + } + return c.NoContent(http.StatusOK)
📜 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 (11)
api/plugin.go(3 hunks)api/server.go(6 hunks)common/vault.go(2 hunks)go.mod(2 hunks)plugin/fees/fees.go(2 hunks)plugin/fees/transaction.go(4 hunks)plugin/payroll/payroll.go(1 hunks)plugin/payroll/policy.go(2 hunks)plugin/payroll/transaction.go(7 hunks)scripts/dev/add_balance/main.go(2 hunks)scripts/dev/create_fee_policy/main.go(2 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
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: 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.
📚 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.goplugin/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/payroll/policy.goplugin/payroll/payroll.goplugin/fees/fees.goplugin/fees/transaction.goplugin/payroll/transaction.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.goplugin/fees/fees.gogo.modapi/plugin.goscripts/dev/create_fee_policy/main.goapi/server.goplugin/payroll/transaction.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-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-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
📚 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.go
🧬 Code Graph Analysis (3)
common/vault.go (1)
scripts/dev/create_fee_policy/main.go (1)
DecryptVaultFromBackup(205-234)
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)
🔇 Additional comments (14)
plugin/fees/fees.go (1)
326-326: Confirm vault filename helper preserves previous namingThe new code in plugin/fees/fees.go (line 326) now delegates to vgcommon.GetVaultBackupFilename (implemented at common/vault.go:11–15), matching all other call sites. Before approving, please:
• Review vgcommon.GetVaultBackupFilename in common/vault.go and ensure its logic matches the former inline derivation in plugin/fees/fees.go.
• Spot-check existing vault files by printing old vs. new filename outputs for one publicKey/pluginID pair to confirm they’re identical.scripts/dev/add_balance/main.go (2)
19-21: Imports migrated to vultisig-go address/common — LGTMMoving to github.com/vultisig/vultisig-go/address and vgcommon aligns with the new module. No functional changes introduced here.
62-62: Address derivation now fed vgcommon.Ethereum — LGTMPassing vgcommon.Ethereum to address.GetAddress matches the new common package. Behavior should be equivalent.
go.mod (1)
146-146: go-wrappers indirect bump notedIndirect upgrade to go-wrappers looks fine and should be compatible.
plugin/payroll/policy.go (2)
10-10: vgcommon import swap — LGTMReplacing rcommon with vgcommon is consistent with the new shared module usage.
33-33: Evaluate now uses vgcommon.Chain — LGTMEngine invocation with vgcommon.Chain maintains expected behavior.
common/vault.go (1)
9-10: Imports migrated to vgcommon — looks good.Using github.com/vultisig/vultisig-go/common aligns with the PR’s direction and reduces coupling with verifier internals.
scripts/dev/create_fee_policy/main.go (1)
22-22: Import switch to vgcommon is correct.This removes reliance on verifier/common and keeps the script aligned with vultisig-go.
api/plugin.go (1)
19-20: Imports migrated to vgcommon — good alignment.This reduces direct dependency on verifier/common and centralizes chain helpers.
plugin/fees/transaction.go (1)
22-24: Imports migrated to vultisig-go/address and vgcommon — good move.This aligns address derivation and chain constants with the shared library.
plugin/payroll/transaction.go (4)
27-29: Imports migrated to vultisig-go/address and vgcommon — LGTM.This standardizes chain/address references on vultisig-go across the plugin.
128-136: Hardcoded chain set to Ethereum — OK per current scope.This mirrors the prior intentional limitation to ETH first. No action needed now.
341-346: genUnsignedTx signature updated to vgcommon.Chain — good change.Switching the parameter type simplifies callers and keeps the switch consistent with vgcommon’s Chain enum.
259-259: Log uses vgcommon.Ethereum.String() — matches the chain variable.No concerns.
3 participants
Migrate to vultisig-go shared library
This PR migrates the codebase from using
github.com/vultisig/verifiertogithub.com/vultisig/vultisig-gofor core address and chain functionality.Summary
This migration consolidates shared vault types and utilities into a centralized
vultisig-golibrary, eliminating code duplication across repositories and providing a unified foundation for vault operations.Key Changes
📦 Import Path Migration
github.com/vultisig/verifier/addresstogithub.com/vultisig/vultisig-go/addressgithub.com/vultisig/verifier/commontogithub.com/vultisig/vultisig-go/common🏦 Vault Types Migration
vultisig-govault types and operations⬆️ Dependency Updates
v0.0.0-20250818095937-af97443fcbbev0.0.0-20250820091538-a29a985cae1d🏷️ Standardized Naming Conventions
vgcommon: Alias forvultisig-go/commonvcommon: Alias forverifier/commonFiles Updated
Core Components
vultisig-gotypes🔗 Related PRs