Conversation
📝 WalkthroughWalkthroughAdds gateway-based authentication to CRETransport: stores a synchronized auth cookie, normalizes endpoints, retries JSON-RPC on 401 by performing gateway auth (SIWE-like message composition, signing, kgw.authn call), and exposes helper JSON-RPC methods that surface response headers. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as CRETransport Client
participant CRE as CRETransport
participant Gateway as Gateway Service
participant RPC as JSON-RPC Endpoint
Note over Client,CRE: Caller issues JSON-RPC Call
Client->>CRE: Call(ctx, method, params)
CRE->>RPC: doJSONRPC(method, params)
RPC-->>CRE: 401 Unauthorized
rect rgb(240, 248, 255)
Note over CRE,Gateway: Gateway authentication flow
CRE->>Gateway: doJSONRPCWithResponse(kgw.getAuthParams)
Gateway-->>CRE: AuthnParameterResponse
CRE->>CRE: composeGatewayAuthMessage(...) — sign message
CRE->>Gateway: doJSONRPCWithResponse(kgw.authn, signedPayload)
Gateway-->>CRE: 200 + Set-Cookie header
CRE->>CRE: store authCookie (authCookieMu)
end
Note over CRE: Retry original call with cookie
CRE->>RPC: doJSONRPC(method, params) + Cookie header
RPC-->>CRE: 200 OK (result)
CRE-->>Client: Return result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
Time Submission Status
|
![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 (3)
core/tnclient/transport_cre.go (3)
115-131: Fragile 401 detection via string matching.Using
strings.Contains(err.Error(), "401")is brittle—this could match unrelated text containing "401" (e.g., error codes, timestamps, or message content). The error fromdoJSONRPCline 182 is formatted as"unexpected HTTP status code: %d", so this works currently, but it's fragile.Consider extracting the status code check more explicitly, or returning a typed error from
doJSONRPCthat includes the status code.🔎 Suggested approach using a typed error
// Define a typed error for HTTP status issues type HTTPStatusError struct { StatusCode int } func (e *HTTPStatusError) Error() string { return fmt.Sprintf("unexpected HTTP status code: %d", e.StatusCode) } // In doJSONRPC: if httpResp.StatusCode != 200 { return &HTTPStatusError{StatusCode: httpResp.StatusCode} } // In callJSONRPC: var httpErr *HTTPStatusError if errors.As(err, &httpErr) && httpErr.StatusCode == 401 { // authenticate and retry }
573-581: Consider using case-insensitive header lookup.HTTP headers are case-insensitive per RFC 7230. The code tries "set-cookie" then "Set-Cookie", but other variations (e.g., "SET-COOKIE") would be missed. Depending on the CRE HTTP client's header normalization, this may or may not be an issue.
🔎 Suggested approach for robust header lookup
// Helper for case-insensitive header lookup func getHeaderCaseInsensitive(headers map[string]string, key string) (string, bool) { lowerKey := strings.ToLower(key) for k, v := range headers { if strings.ToLower(k) == lowerKey { return v, true } } return "", false } // Usage: setCookie, ok := getHeaderCaseInsensitive(authResp, "Set-Cookie")
598-651: Code duplication withdoJSONRPC.
doJSONRPCWithResponseduplicates most of the logic fromdoJSONRPC(request building, sending, response parsing). Consider extracting the common logic into a shared helper that returns both the response body and headers, allowing callers to choose what they need.This is not urgent but would reduce maintenance burden.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/tnclient/transport_cre.go
🧰 Additional context used
🧬 Code graph analysis (1)
core/tnclient/transport_cre.go (1)
core/contractsapi/stream.go (1)
Action(15-17)
⏰ 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: test
🔇 Additional comments (3)
core/tnclient/transport_cre.go (3)
64-65: LGTM!Good use of
sync.RWMutexfor protecting concurrent access toauthCookie. The read-write lock allows multiple concurrent readers during normal operation while ensuring exclusive access during authentication updates.
159-164: LGTM!Correct use of
RLock/RUnlockfor reading the auth cookie, allowing concurrent reads while writes are exclusive.
5-26: LGTM!All new imports (
bytes,net/url,gateway) are appropriately used for the authentication functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
core/tnclient/transport_cre.go (1)
657-678: The custom authentication message format is correct.Based on learnings, the gateway authentication uses a custom SIWE-like format (not standard SIWE) that intentionally:
- Omits the account address line after "sign in with your account:"
- Uses "Issue At" instead of "Issued At"
This matches the kwil-db gateway's expectations. The current implementation is correct.
🧹 Nitpick comments (2)
core/tnclient/transport_cre.go (2)
113-136: Consider more robust 401 detection.The current implementation detects 401 errors via string matching on the error message (
strings.Contains(err.Error(), "401")). While this works with the current error format from line 184, it's fragile and will break if the error message format changes.🔎 Potential improvements
Option 1: Modify
doJSONRPCto return a typed error that includes the HTTP status code:type HTTPError struct { StatusCode int Message string } func (e *HTTPError) Error() string { return fmt.Sprintf("HTTP %d: %s", e.StatusCode, e.Message) }Option 2: Have
doJSONRPCreturn both error and status code:func (t *CRETransport) doJSONRPC(...) (statusCode int, err error)Then in
callJSONRPC:statusCode, err := t.doJSONRPC(ctx, method, params, result) if err != nil && statusCode == 401 { // authenticate and retry }
587-594: Consider validating the extracted cookie value.The code splits the
Set-Cookieheader by semicolon and takes the first part, but doesn't validate that this part is non-empty. If the header is malformed (e.g., just semicolons), this could store an empty string as the cookie.🔎 Proposed enhancement
if ok && setCookie != "" { // Parse the cookie (just extract the name=value part) cookieParts := strings.Split(setCookie, ";") - if len(cookieParts) > 0 { + if len(cookieParts) > 0 && strings.TrimSpace(cookieParts[0]) != "" { t.authCookieMu.Lock() - t.authCookie = cookieParts[0] // Store just the name=value part + t.authCookie = strings.TrimSpace(cookieParts[0]) // Store just the name=value part t.authCookieMu.Unlock() + } else { + return fmt.Errorf("malformed Set-Cookie header (empty cookie value): %s", setCookie) } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/tnclient/transport_cre.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-24T07:10:07.366Z
Learnt from: MicBun
Repo: trufnetwork/sdk-go PR: 166
File: core/tnclient/transport_cre.go:657-678
Timestamp: 2025-12-24T07:10:07.366Z
Learning: In the trufnetwork/sdk-go repository, the gateway authentication in `core/tnclient/transport_cre.go` uses a custom SIWE-like format (not standard SIWE). The custom format intentionally omits the account address line and uses "Issue At" instead of "Issued At" to match the kwil-db gateway's expectations.
Applied to files:
core/tnclient/transport_cre.go
🧬 Code graph analysis (1)
core/tnclient/transport_cre.go (1)
core/contractsapi/stream.go (1)
Action(15-17)
⏰ 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: test
🔇 Additional comments (5)
core/tnclient/transport_cre.go (5)
89-94: LGTM! Endpoint normalization now handles edge cases correctly.The use of
TrimRightremoves all trailing slashes before checking for the/rpc/v1suffix, which properly handles cases like"https://example.com/rpc/v1/"that were flagged in previous reviews.
156-166: LGTM! Cookie injection is concurrency-safe.The read-lock pattern correctly protects access to
authCookieand releases the lock before the potentially long-running HTTP request.
218-256: LGTM! Call method properly encodes inputs and creates authenticated call messages.The implementation correctly:
- Encodes inputs using
types.EncodeValue- Defaults to "main" namespace following TRUF.NETWORK conventions
- Passes the signer to support authenticated gateway calls (as explained in the comment)
- Uses the new
callJSONRPCwhich handles 401 retry
602-655: LGTM! Method correctly omits auth cookie for authentication flow.The method is used for
kgw.authn_paramandkgw.authncalls, which must not include the auth cookie (since we're establishing authentication). The implementation correctly returns headers forSet-Cookieextraction.If this method is reused outside the authentication flow in the future, consider documenting that it does not include the auth cookie, unlike
doJSONRPC.
64-65: LGTM! Appropriate use of RWMutex for auth cookie storage.Using
RWMutexis the right choice here since the cookie is read on every request but written only during authentication.
1 participant
resolves: https://github.com/truflation/website/issues/3074
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.