feat: add non-custodial signing flow (key_mode, prepare/execute API, external registration)#152
feat: add non-custodial signing flow (key_mode, prepare/execute API, external registration)#152
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly advances the non-custodial signing feature by introducing the necessary infrastructure for external key management during token transfers. It includes database schema changes to track user key modes, extends the Canton SDK with new Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant features for non-custodial signing, including PrepareTransfer and ExecuteTransfer methods in the Canton SDK, a new in-memory cache for prepared transfers, and database schema updates for user key management. However, it introduces a potential Denial of Service (DoS) vector due to an unbounded in-memory cache and an Information Disclosure risk (IDOR) in the PrepareTransfer method due to a lack of authorization checks. Additionally, there are suggestions to enhance consistency and robustness by adding validation methods to the new request types, aligning them with existing patterns in the codebase.
| func (c *Client) PrepareTransfer(ctx context.Context, req *PrepareTransferRequest) (*PreparedTransfer, error) { | ||
| if c.preparedCache == nil { | ||
| return nil, fmt.Errorf("prepared transfer cache not configured") | ||
| } | ||
| if req.FromPartyID == "" || req.ToPartyID == "" { | ||
| return nil, fmt.Errorf("from/to party is required") | ||
| } | ||
| if req.Amount == "" { | ||
| return nil, fmt.Errorf("amount is required") | ||
| } | ||
| if req.TokenSymbol == "" { | ||
| return nil, fmt.Errorf("token symbol is required") | ||
| } | ||
|
|
||
| holdings, err := c.GetHoldings(ctx, req.FromPartyID, req.TokenSymbol) |
There was a problem hiding this comment.
The PrepareTransfer method has an Insecure Direct Object Reference (IDOR) vulnerability. It allows specifying any FromPartyID and fetches holdings without verifying if the authenticated user is authorized for that FromPartyID, which could allow an attacker to probe token holdings. To address this, implement an authorization check within a validate() method for PrepareTransferRequest to ensure the FromPartyID matches the authenticated user. This also aligns with existing patterns for consistency and robustness, similar to other request types like MintRequest and BurnRequest.
func (c *Client) PrepareTransfer(ctx context.Context, req *PrepareTransferRequest) (*PrepareTransferResponse, error) {
// TODO: Add authorization check for FromPartyID
// Existing code for PrepareTransfer
// ...
return &PrepareTransferResponse{/* ... */}, nil
}
pkg/cantonsdk/token/client.go
Outdated
| func (c *Client) ExecuteTransfer(ctx context.Context, req *ExecuteTransferRequest) error { | ||
| if c.preparedCache == nil { | ||
| return fmt.Errorf("prepared transfer cache not configured") | ||
| } |
There was a problem hiding this comment.
To improve robustness and consistency, consider adding a validate() method to ExecuteTransferRequest to ensure all required fields are present before use. This helps fail early with clear error messages.
You can add the following method to pkg/cantonsdk/token/types.go:
func (r *ExecuteTransferRequest) validate() error {
if r.TransferID == "" {
return fmt.Errorf("transfer ID is required")
}
if len(r.Signature) == 0 {
return fmt.Errorf("signature is required")
}
if r.SignedBy == "" {
return fmt.Errorf("signed by is required")
}
return nil
}Then, you can call it here at the beginning of the function.
if err := req.validate(); err != nil {
return fmt.Errorf("invalid request: %w", err)
}
salindne
force-pushed
the
feature/111-a-db-and-b-sdk
branch
from
8539a96 to
ff8cd65
Compare
Add database migration for key_mode and canton_public_key_fingerprint columns. Extend User domain model with KeyMode and NewExternal constructor for non-custodial users. Add CategoryGone error type. Canton SDK: add PrepareTransfer/ExecuteTransfer to Token interface with in-memory PreparedTransferCache (TTL + atomic GetAndDelete for replay prevention). Extract buildTransferCommand helper from transferViaFactory. Add CompressedKeyToSPKI helper for secp256k1 public key encoding.
salindne
force-pushed
the
feature/111-a-db-and-b-sdk
branch
from
ff8cd65 to
33e124a
Compare
Move cache responsibility out of the SDK client — PrepareTransfer now returns the result without caching, and ExecuteTransfer accepts the full PreparedTransfer in the request. The service layer (added in next PR) will own the cache lifecycle. Also add validate() methods to PrepareTransferRequest and ExecuteTransferRequest for defense-in-depth input checking.
New pkg/transfer package with thin service layer for the two-step non-custodial transfer flow: - POST /api/v2/transfer/prepare — validates inputs, resolves parties, calls SDK PrepareTransfer, returns transaction hash for client signing - POST /api/v2/transfer/execute — verifies fingerprint match, decodes DER signature, calls SDK ExecuteTransfer Auth uses existing EVM signature verification (X-Signature/X-Message) with timed message validation (5-minute replay window). Input validation: EVM address format, positive decimal amount, token allowlist (DEMO/PROMPT). Rejects custodial users with 400. Wire transfer routes and cache into api/server.go.
- TransferService now owns the PreparedTransferCache (SDK is stateless) - Allowed token symbols are derived from token config instead of hardcoded - readJSON uses json.NewDecoder with DisallowUnknownFields for stricter parsing
Support non-custodial (external) user registration via two HTTP calls: 1. POST /register/prepare-topology — verify EIP-191 signature, validate whitelist, derive SPKI public key from compressed secp256k1 key, call Canton GenerateExternalPartyTopology, return topology hash + fingerprint + registration token for client-side signing. 2. POST /register (key_mode=external) — retrieve pending topology from cache, verify SPKI key consistency between steps, call Canton AllocateExternalPartyWithSignature with client's DER signature, create fingerprint mapping, save user as key_mode=external. Canton SDK: split AllocateExternalParty into GenerateExternalPartyTopology and AllocateExternalPartyWithSignature for the two-step flow. Add TopologyCache (in-memory, 5-min TTL) for pending registrations. Add PrepareTopologyResponse and external fields to RegisterRequest. Extend logging decorator with PrepareExternalRegistration wrapper.
The whitelist check was after the KeyModeExternal early return, meaning external registrations bypassed it. While PrepareExternalRegistration also checks the whitelist, defense-in-depth requires the check at both entry points.
Integration test script (test-prepare-execute.go) covering: - Happy path: register external user, mint, prepare, sign, execute - Expired transfer (past TTL → 410) - Replay prevention (double execute → 404) - Wrong fingerprint (→ 403) - Custodial rejection (→ 400) Add docs/non-custodial-snap-testing.md with complete Snap integration guide: cryptographic requirements, API reference, error tables, RPC methods, JavaScript dapp example, and curl testing commands. Update e2e-local config with test user addresses.
…ment Update mock expectations in registration tests to match the new whitelist-before-UserExists ordering. Also remove no-op `_ = hash` in test script.
salindne
force-pushed
the
feature/111-a-db-and-b-sdk
branch
from
d763330 to
e9b0910
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces significant functionality for non-custodial signing by adding PrepareTransfer and ExecuteTransfer methods to the Canton SDK, along with an in-memory cache for prepared transfers to enhance security. The database schema is appropriately updated to distinguish between custodial and external key modes. The changes are well-implemented, including necessary domain model updates, database migrations, and unit tests for the new caching logic. My feedback includes a couple of suggestions to remove redundant code for improved maintainability.
pkg/userstore/model.go
Outdated
| if usr.KeyMode == "" { | ||
| usr.KeyMode = user.KeyModeCustodial | ||
| } |
There was a problem hiding this comment.
The database migration 11_add_key_mode.go defines the key_mode column as NOT NULL DEFAULT 'custodial'. This ensures that dao.KeyMode will never be an empty string when a UserDao is populated from the database. Therefore, this check is redundant and can be safely removed.
There was a problem hiding this comment.
Fixed in the same commit — removed from both toUserDao and toUser.
|
/gemini review |
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
|
/gemini review |
salindne
changed the title
There was a problem hiding this comment.
Code Review
This pull request introduces a significant feature for non-custodial transfers, including a two-step prepare/execute flow, database schema updates, and new API endpoints. The implementation is comprehensive, with new services, caches, and extensive documentation. My review has identified a critical issue related to the handling of transfer expiration TTLs, where a hardcoded value in the SDK overrides the configured cache TTL. I've also noted a couple of areas in the new documentation that could be clarified to prevent ambiguity for developers. Overall, this is a well-structured addition, and addressing these points will enhance its robustness and clarity.
- Move ExpiresAt assignment from SDK to cache.Put() so TTL is controlled by the cache owner (2min) instead of SDK (1h) - Add maxSize cap (10k) to PreparedTransferCache to prevent OOM - cache.Put() now returns error when cache is full - Remove redundant KeyMode defaulting in userstore model (DB default handles it) - Remove migration 11_add_key_mode.go (not in prod, schema created from struct) - Fix docs: clarify which fingerprint is used for signed_by
…rator - Extract TopologyCacheProvider interface from concrete TopologyCache - Export PendingTopology to user package (avoids import cycle with mocks) - Extract TransferCache interface from concrete PreparedTransferCache - Extract transfer.Service interface from TransferService - Add transfer/log.go logging decorator (matches user/service/log.go pattern) - Wire transfer.NewLog decorator in API server setup - Add TopologyCacheProvider mock for testing
- PrepareExternalRegistration: missing public key, user exists, not whitelisted - TransferService.Prepare: success, missing fields, sender not found, sender not external, unsupported token - TransferService.Execute: success, missing fields, transfer not found, transfer expired
- Move field-presence checks (empty To/Amount/Token, empty TransferID/ Signature/SignedBy) from transfer service to http.go - Move external registration field checks (RegistrationToken, TopologySignature, CantonPublicKey) from service to http.go - Remove duplicate CantonPublicKey check in PrepareExternalRegistration (already validated in HTTP handler) - Keep domain validation (allowed token symbols, key mode checks) in service layer - Fix lint: extract testMessage constant, embed token.Token interface in mock, fix goimports ordering, break long line in server.go
|
/gemini review |
There was a problem hiding this comment.
Code Review
The pull request introduces a comprehensive non-custodial signing flow, including two-step registration and transfer APIs. The changes span across the domain model, Canton SDK, API handlers, and include new caching mechanisms and an integration test script. The documentation for the new flow is clear and detailed. The implementation demonstrates good practices for input validation, authentication, and logging of sensitive data. Overall, the feature is well-implemented and tested.
| const defaultRequestTimeout = 60 | ||
| const ( | ||
| defaultRequestTimeout = 60 | ||
| topologyCacheTTL = 5 * time.Minute |
There was a problem hiding this comment.
The topologyCacheTTL is a magic number. It would be better to define this as a configurable value (e.g., in the config.yaml) or as a clearly named constant if it's a fixed domain-specific requirement, with a comment explaining its significance.
| defer stopReconcile() | ||
|
|
||
| topologyCache := userservice.NewTopologyCache(topologyCacheTTL) | ||
| go topologyCache.Start(ctx) |
There was a problem hiding this comment.
The topologyCache.Start(ctx) is launched as a goroutine without explicit panic recovery. While the Start method itself handles context cancellation, a panic originating from within the cleanup method or any other part of this goroutine could lead to an unhandled crash of the application. Consider adding a defer with recover() to log and handle such panics gracefully.
|
|
||
| router := s.setupRouter(evmStore, cantonClient, tokenService, userservice.NewLog(registrationService, logger), logger) | ||
| transferSvc := transfer.NewTransferService(cantonClient.Token, userStore, tokenSymbols(cfg.Token)) | ||
| go transferSvc.StartCache(ctx) |
There was a problem hiding this comment.
Similar to the topologyCache, the transferSvc.StartCache(ctx) is launched as a goroutine without explicit panic recovery. A panic within this goroutine could lead to an unhandled application crash. It's recommended to add a defer with recover() to ensure graceful handling of unexpected panics.
| ErrCacheFull = errors.New("cache is full") | ||
| ) | ||
|
|
||
| const defaultCleanupInterval = 30 * time.Second |
There was a problem hiding this comment.
The defaultCleanupInterval is a magic number. It would be better to define this as a configurable value or as a clearly named constant with a comment explaining its significance.
|
|
||
| // Start runs a background goroutine that periodically removes expired entries. | ||
| // It stops when the context is canceled. | ||
| func (c *PreparedTransferCache) Start(ctx context.Context) { |
There was a problem hiding this comment.
The Start method launches a goroutine without explicit panic recovery. While the cleanup logic itself appears safe, a panic originating from within this goroutine could lead to an unhandled application crash. Consider adding a defer with recover() to log and handle such panics gracefully.
|
|
||
| const ( | ||
| maxRequestBodyBytes = 1 << 20 // 1MB | ||
| messageMaxAge = 5 * time.Minute |
There was a problem hiding this comment.
The messageMaxAge constant is a magic number. It would be better to define this as a configurable value or as a clearly named constant with a comment explaining its significance.
|
|
||
| const ( | ||
| transferServiceName = "TransferService" | ||
| signatureDisplaySize = 16 |
There was a problem hiding this comment.
The signatureDisplaySize constant is a magic number. It would be better to define this as a clearly named constant with a comment explaining its significance.
| defaultCacheTTL = 2 * time.Minute | ||
| defaultCacheMaxSize = 10000 |
There was a problem hiding this comment.
The defaultCacheTTL and defaultCacheMaxSize constants are magic numbers. It would be better to define these as configurable values (e.g., in the config.yaml) or as clearly named constants with comments explaining their significance.
| defaultTopologyTTL = 5 * time.Minute | ||
| topologyCleanupInterval = 30 * time.Second |
There was a problem hiding this comment.
The defaultTopologyTTL and topologyCleanupInterval constants are magic numbers. It would be better to define these as configurable values or as clearly named constants with comments explaining their significance.
| } | ||
|
|
||
| // Start runs a background goroutine that periodically removes expired entries. | ||
| func (c *TopologyCache) Start(ctx context.Context) { |
There was a problem hiding this comment.
The Start method launches a goroutine without explicit panic recovery. While the cleanup logic itself appears safe, a panic originating from within this goroutine could lead to an unhandled application crash. Consider adding a defer with recover() to log and handle such panics gracefully.
elliptic.UnmarshalCompressed does not work with go-ethereum's secp256k1 curve implementation, causing all external registration requests to fail with "invalid canton_public_key". Switch to crypto.DecompressPubkey which is consistent with the rest of the codebase. Also fix compile errors in test-prepare-execute.go (double pointer on Database config, non-existent canton.NewFromAppConfig function).
2 participants
Summary
Complete non-custodial signing feature (#111). Combines all sub-issues (#131–#135).
Database & Domain Model
key_mode(VARCHAR, defaultcustodial) andcanton_public_key_fingerprint(TEXT) columns touserstableKeyMode,CantonPublicKeyFingerprintfields,NewExternal()constructor, key mode constantsCanton SDK (stateless)
PrepareTransfer/ExecuteTransfertoTokeninterfacevalidate()methods on request types,CompressedKeyToSPKIfor secp256k1 SPKI encodingCategoryGone(HTTP 410) for expired resourcesNon-Custodial Transfer API
POST /api/v2/transfer/prepare— builds Canton transaction, returns hash for external signingPOST /api/v2/transfer/execute— completes transfer with client-provided DER signatureTransferServiceownsPreparedTransferCache(TTL + atomicGetAndDeletefor replay prevention)DisallowUnknownFields()Two-Step External Registration
POST /register/prepare-topology— returns topology transaction hash for external signingPOST /registerwithkey_mode=external— completes registration with Canton topology signatureTopologyCachefor caching pending topology transactionsIntegration Test & Docs
scripts/testing/test-prepare-execute.go— end-to-end prepare/execute test scriptdocs/non-custodial-snap-testing.md— Snap testing guideTest plan
go test ./...— all tests passmake lint— passesgo run ./cmd/api-server/migrate/main.go -config config.yaml upgo run scripts/testing/test-prepare-execute.go