feat: add non-custodial transfer HTTP API (#133)#153
feat: add non-custodial transfer HTTP API (#133)#153salindne merged 9 commits intofeature/111-a-db-and-b-sdkfrom
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 introduces the foundational HTTP API for non-custodial token transfers, enabling users to securely initiate and complete transactions by signing them externally. It establishes a robust two-step process, enhancing security and user control over their assets, and integrates necessary authentication and validation mechanisms into the existing API infrastructure. 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 a non-custodial transfer API with prepare and execute endpoints, implemented with a new transfer package for service and HTTP layers. While the overall design is sound, a critical security concern has been identified regarding the authentication mechanism: the lack of binding between the EIP-191 signature and the request body. This vulnerability could lead to request manipulation and Denial of Service (DoS) attacks. It is recommended to strengthen the authentication scheme by including a hash of the request body and enforcing action-specific prefixes in the signed messages. Further improvements include enhancing the robustness of JSON request handling and making the token allowlist configurable for better maintainability.
| func authenticateEVM(r *http.Request) (string, error) { | ||
| sig := r.Header.Get("X-Signature") | ||
| msg := r.Header.Get("X-Message") | ||
| if sig == "" || msg == "" { | ||
| return "", apperrors.UnAuthorizedError(nil, "authentication required") | ||
| } | ||
|
|
||
| if err := auth.ValidateTimedMessage(msg, messageMaxAge); err != nil { | ||
| return "", apperrors.UnAuthorizedError(err, "message expired or invalid format") | ||
| } | ||
|
|
||
| recovered, err := auth.VerifyEIP191Signature(msg, sig) | ||
| if err != nil { | ||
| return "", apperrors.UnAuthorizedError(err, "invalid signature") | ||
| } | ||
|
|
||
| return auth.NormalizeAddress(recovered.Hex()), nil | ||
| } |
There was a problem hiding this comment.
The authenticateEVM function verifies the caller's identity using an EIP-191 signature over a message that only contains a timestamp. This has two security implications:
- Missing Request Body Binding: The signature does not cover the HTTP request body, allowing an attacker to replay a valid signature with modified request parameters (e.g., different recipient or amount in
Prepare) on behalf of the user. - Missing Action Prefix Validation: Although the documentation suggests a prefix like
transfer:should be used, the code inauth.ValidateTimedMessagedoes not enforce it. This allows a signature intended for one action to be replayed for another if they use the same authentication scheme.
While the transfer flow is partially protected by a second layer of signing for the Canton transaction, this authentication pattern is fundamentally insecure and could lead to more severe vulnerabilities if applied to other endpoints.
| r.Post("/api/v2/transfer/prepare", apphttp.HandleError(h.prepare)) | ||
| r.Post("/api/v2/transfer/execute", apphttp.HandleError(h.execute)) |
There was a problem hiding this comment.
The Prepare endpoint creates a new PreparedTransfer entry in an in-memory cache for every valid request. Because the authenticateEVM function allows replaying the same X-Signature and X-Message multiple times within a 5-minute window, an attacker can capture a single valid signature and use it to flood the server with a large number of Prepare requests. Since the PreparedTransferCache has no size limit, this can lead to memory exhaustion and a Denial of Service (DoS).
| func readJSON(r *http.Request, dst any) error { | ||
| body, err := io.ReadAll(io.LimitReader(r.Body, maxRequestBodyBytes)) | ||
| if err != nil { | ||
| return apperrors.BadRequestError(err, "failed to read request") | ||
| } | ||
| if err := json.Unmarshal(body, dst); err != nil { | ||
| return apperrors.BadRequestError(err, "invalid JSON") | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The current implementation of readJSON reads the entire request body into memory before unmarshalling. A more memory-efficient and robust approach is to use json.NewDecoder on the request body stream. This avoids a large memory allocation for the request body.
Additionally, calling dec.DisallowUnknownFields() makes the API stricter by rejecting requests with unknown JSON fields, which can help catch client-side bugs. Checking dec.More() ensures that there is no extraneous data after the main JSON object, further improving robustness.
func readJSON(r *http.Request, dst any) error {
dec := json.NewDecoder(io.LimitReader(r.Body, maxRequestBodyBytes))
dec.DisallowUnknownFields()
if err := dec.Decode(dst); err != nil {
return apperrors.BadRequestError(err, "invalid JSON")
}
if dec.More() {
return apperrors.BadRequestError(nil, "request body contains extra data")
}
return nil
}
pkg/transfer/service.go
Outdated
| // allowedTokenSymbols defines the set of valid token symbols for transfers. | ||
| var allowedTokenSymbols = map[string]bool{ | ||
| "DEMO": true, | ||
| "PROMPT": true, | ||
| } |
There was a problem hiding this comment.
The list of allowed token symbols is currently hardcoded. For better maintainability and flexibility, consider making this list configurable. This could be achieved by passing the allowed symbols from the application's configuration into the TransferService during its creation. This would allow operators to update the list of supported tokens without requiring a code change and redeployment.
There was a problem hiding this comment.
@salindne we can take it from token config.
Where we already have supported tokens
There was a problem hiding this comment.
Fixed — TransferService now takes allowed symbols from token config via NewTransferService(cantonToken, userStore, allowedSymbols). The symbols are extracted from cfg.Token.SupportedTokens in server.go.
pkg/transfer/service.go
Outdated
| // allowedTokenSymbols defines the set of valid token symbols for transfers. | ||
| var allowedTokenSymbols = map[string]bool{ | ||
| "DEMO": true, | ||
| "PROMPT": true, | ||
| } |
There was a problem hiding this comment.
@salindne we can take it from token config.
Where we already have supported tokens
salindne
force-pushed
the
feature/111-a-db-and-b-sdk
branch
from
8539a96 to
ff8cd65
Compare
salindne
force-pushed
the
feature/111-c-http-api
branch
from
8b68b03 to
5e10849
Compare
salindne
force-pushed
the
feature/111-a-db-and-b-sdk
branch
from
ff8cd65 to
33e124a
Compare
salindne
force-pushed
the
feature/111-c-http-api
branch
2 times, most recently
from
90ad5d9 to
7c80a3f
Compare
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
salindne
force-pushed
the
feature/111-c-http-api
branch
from
7c80a3f to
75e867d
Compare
2 participants
Summary
Sub-issue C (#133) of the non-custodial signing feature (#111).
pkg/transfer/package: Thin service layer for two-step non-custodial transfer flowPOST /api/v2/transfer/prepare— validate inputs, resolve sender/recipient parties, call SDKPrepareTransfer, return transaction hash for client signingPOST /api/v2/transfer/execute— verify fingerprint match, decode DER signature, call SDKExecuteTransferX-Signature/X-Messageheaders with timed message validation (5-minute replay window)shopspring/decimal), token allowlist (DEMO/PROMPT)api/server.goTest plan
go build ./...— compilesgo test ./...— all tests passmake lint— passescurl -X POST /api/v2/transfer/preparewith custodial user → 400Stacked PR 2/4 for #111 — depends on #152