Conversation
WalkthroughAdds a CRE-based Transport implementation and two client option constructors for CRE, plus tests, Go toolchain and dependency bumps, and documentation text updates changing network naming to "TRUF.NETWORK". Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CRETransport
participant Signer
participant CRE_HTTP as "CRE HTTP Client"
participant Chain
rect rgb(200,220,255)
Note over Client,CRETransport: Read path — Call
Client->>CRETransport: Call(ctx, namespace, action, inputs)
CRETransport->>CRETransport: build JSON-RPC params
CRETransport->>CRE_HTTP: POST user.call
CRE_HTTP->>Chain: forward request
Chain-->>CRE_HTTP: response
CRETransport->>CRETransport: validate & unmarshal JSON-RPC
CRETransport-->>Client: CallResult
end
rect rgb(220,250,220)
Note over Client,Signer: Write path — Execute
Client->>CRETransport: Execute(ctx, namespace, action, inputs, opts)
CRETransport->>CRETransport: fetch ChainID (lazy, cached)
CRETransport->>Signer: sign transaction payload
Signer-->>CRETransport: signature
CRETransport->>CRE_HTTP: POST user.broadcast (signed tx)
CRE_HTTP->>Chain: broadcast tx
Chain-->>CRE_HTTP: txHash
CRETransport-->>Client: txHash
end
rect rgb(255,240,200)
Note over Client,CRETransport: Polling — WaitTx
Client->>CRETransport: WaitTx(ctx, txHash, interval)
loop poll at interval
CRETransport->>CRE_HTTP: POST user.tx_query
CRE_HTTP->>Chain: query tx status
Chain-->>CRE_HTTP: TxQueryResponse
CRETransport->>CRETransport: check Height / transient errors
end
CRETransport-->>Client: final TxQueryResponse
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 1
🧹 Nitpick comments (5)
core/tnclient/options_cre.go (2)
74-80: Silent error suppression may hide transport creation failures.The error from
NewCRETransportis discarded with_. WhileNewCRETransportcurrently always returnsnilerror, this pattern can hide failures if the constructor is modified in the future to return errors.Consider either:
- Logging the error if it occurs
- Panicking on error (since this is configuration-time)
- Storing the error and surfacing it later during
NewClient🔎 Proposed fix to handle the error
func WithCRETransport(runtime cre.NodeRuntime, endpoint string) Option { return func(c *Client) { // Note: Transport is created immediately with the current signer (if set) // If WithSigner is applied after this option, the signer won't be available yet // For guaranteed signer availability, use WithCRETransportAndSigner instead - c.transport, _ = NewCRETransport(runtime, endpoint, c.signer) + transport, err := NewCRETransport(runtime, endpoint, c.signer) + if err != nil { + // Store error to be returned by NewClient, or panic since this is config-time + panic(fmt.Sprintf("failed to create CRE transport: %v", err)) + } + c.transport = transport } }
101-108: Same silent error suppression concern.Apply the same error handling improvement here for consistency.
🔎 Proposed fix to handle the error
func WithCRETransportAndSigner(runtime cre.NodeRuntime, endpoint string, signer auth.Signer) Option { return func(c *Client) { // Set signer first c.signer = signer // Then create CRE transport with the signer - c.transport, _ = NewCRETransport(runtime, endpoint, signer) + transport, err := NewCRETransport(runtime, endpoint, signer) + if err != nil { + panic(fmt.Sprintf("failed to create CRE transport: %v", err)) + } + c.transport = transport } }core/tnclient/transport_cre.go (3)
274-310: First transaction check is delayed by one interval.The ticker-based polling waits for the first tick before checking transaction status, introducing an unnecessary delay equal to the polling interval. Consider checking immediately before entering the poll loop:
🔎 Proposed fix for immediate first check
func (t *CRETransport) WaitTx(ctx context.Context, txHash types.Hash, interval time.Duration) (*types.TxQueryResponse, error) { ticker := time.NewTicker(interval) defer ticker.Stop() for { + // Query transaction status + params := map[string]any{ + "tx_hash": txHash, + } + + var result types.TxQueryResponse + if err := t.callJSONRPC(ctx, "user.tx_query", params, &result); err != nil { + errMsg := err.Error() + isTransient := containsAny(errMsg, []string{"not found", "not indexed", "pending", "unknown transaction"}) + if !isTransient { + return nil, fmt.Errorf("transaction query failed: %w", err) + } + } else if result.Height > 0 { + return &result, nil + } + select { case <-ctx.Done(): return nil, ctx.Err() case <-ticker.C: - // Query transaction status - params := map[string]any{ - "tx_hash": txHash, - } - - var result types.TxQueryResponse - if err := t.callJSONRPC(ctx, "user.tx_query", params, &result); err != nil { - // ... error handling - } - - if result.Height > 0 { - return &result, nil - } + // Continue to next iteration for retry } } }
312-343: Consider using standard library for case-insensitive matching.The custom implementation works but could be simplified with
strings.ToLowerandstrings.Contains. If there's a specific reason to avoid the strings package in WASI (allocations, build constraints), please add a comment explaining the choice.🔎 Simplified alternative
+import "strings" + // containsAny checks if a string contains any of the specified substrings (case-insensitive) func containsAny(s string, substrings []string) bool { - lowerS := s + lowerS := strings.ToLower(s) for _, substr := range substrings { - if len(substr) == 0 { - continue - } - // Simple case-insensitive substring check - for i := 0; i <= len(lowerS)-len(substr); i++ { - match := true - for j := 0; j < len(substr); j++ { - c1 := lowerS[i+j] - c2 := substr[j] - // Convert to lowercase for comparison - if c1 >= 'A' && c1 <= 'Z' { - c1 += 'a' - 'A' - } - if c2 >= 'A' && c2 <= 'Z' { - c2 += 'a' - 'A' - } - if c1 != c2 { - match = false - break - } - } - if match { - return true - } + if strings.Contains(lowerS, strings.ToLower(substr)) { + return true } } return false }
368-377: Usingcontext.Background()limits caller control.This method uses
context.Background()insidesync.Once, meaning callers cannot control timeout or cancellation for the chain ID fetch. Since this shareschainIDOncewithExecute, whichever method is called first determines the fetch behavior. Consider accepting a context parameter or documenting this behavior.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
core/tnclient/options_cre.gocore/tnclient/transport.gocore/tnclient/transport_cre.gocore/tnclient/transport_cre_test.godocs/api-reference.mddocs/stream-permissions.mdexamples/stream_cache_demo/go.modgo.modtests/integration/composed_actions_test.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-10T15:39:48.576Z
Learnt from: MicBun
Repo: trufnetwork/sdk-go PR: 138
File: examples/cache_example.go:21-21
Timestamp: 2025-07-10T15:39:48.576Z
Learning: For example code in the trufnetwork/sdk-go repository, the maintainer MicBun prefers to keep examples simple and focused on demonstrating functionality rather than implementing production-ready security practices like environment variables for sensitive values.
Applied to files:
tests/integration/composed_actions_test.gogo.modcore/tnclient/transport_cre_test.godocs/api-reference.mdexamples/stream_cache_demo/go.mod
🧬 Code graph analysis (3)
core/tnclient/options_cre.go (2)
core/tnclient/client.go (1)
Option(29-29)core/tnclient/transport_cre.go (1)
NewCRETransport(81-89)
core/tnclient/transport_cre_test.go (3)
core/tnclient/transport_cre.go (2)
NewCRETransport(81-89)CRETransport(50-59)core/tnclient/transport.go (1)
Transport(38-95)core/tnclient/options_cre.go (2)
WithCRETransport(74-81)WithCRETransportAndSigner(101-109)
core/tnclient/transport_cre.go (2)
core/tnclient/transport.go (1)
Transport(38-95)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 (18)
core/tnclient/transport.go (1)
12-12: Documentation branding update looks good.Consistent terminology update to "TRUF.NETWORK" aligns with the broader PR's documentation changes.
tests/integration/composed_actions_test.go (1)
16-16: Documentation branding update looks good.Consistent terminology update to "TRUF.NETWORK (TN)" aligns with the broader PR's documentation changes.
docs/api-reference.md (1)
175-175: Documentation branding update looks good.Consistent terminology update to "TRUF.NETWORK" aligns with other documentation changes in this PR.
docs/stream-permissions.md (1)
142-160: Documentation improvements look good.The branding update and new sections for "How to get access" and "SDK Role Management API" provide clear guidance for partners and advanced users.
go.mod (2)
3-3: Verify Go version 1.25.3 exists.Same concern as the example module - Go 1.25.3 may not be a released version.
12-13: CRE SDK dependencies are correctly added.Both CRE SDK dependencies are properly specified in go.mod to support the new CRE transport feature. The versions v1.1.2 and v0.10.0 are valid and publicly available.
core/tnclient/options_cre.go (1)
1-8: Build tag and imports look correct.The
wasip1build tag appropriately restricts this file to WASM environments where CRE runs. Imports are minimal and correct.core/tnclient/transport_cre_test.go (4)
15-27: Basic structural test for CRE transport constructor.The test appropriately documents that full integration testing requires the CRE environment. The approach of verifying the function signature exists is reasonable.
29-38: Compile-time interface check is good practice.This documents and verifies that
CRETransportimplementsTransport.
67-185: Example functions contain only comments.The example functions have all code inside multi-line comments (
/* ... */). While this serves as documentation, these won't be executed as Go examples bygo test. This is intentional per the comment noting CRE environment requirement.
40-63: Option function tests verify API surface.These tests ensure the option constructors exist and return the correct type. The implementations safely store the runtime parameter without dereferencing it during option application, so passing
nilin the tests does not cause issues at this stage.examples/stream_cache_demo/go.mod (1)
3-3: No action required. Go 1.25.3 is a valid, officially released version (released October 13, 2025).core/tnclient/transport_cre.go (6)
23-59: Well-structured transport with clear documentation.The struct is well-designed with appropriate concurrency primitives (
sync.Oncefor lazy chainID init,atomic.Uint64for request IDs). The usage example in the doc comment is helpful for CRE workflow integration.
61-89: LGTM!Compile-time interface verification is good practice. The constructor is appropriately simple, and returning an error for future extensibility is acceptable.
97-160: Solid JSON-RPC implementation.The error handling is comprehensive, covering marshal failures, HTTP errors, JSON-RPC errors, and version validation. Using HTTP 200 as the only success code is correct for JSON-RPC 2.0 specification.
162-181: LGTM!Clean implementation of the read-only Call method matching the Transport interface.
345-361: LGTM!The fetch implementation is straightforward. The sync.Once usage concern was addressed in the Execute method review.
379-384: LGTM!Simple accessor correctly implements the Transport interface.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
core/tnclient/transport_cre.go (4)
81-89: Add parameter validation and reconsider error return.The constructor has two issues:
- It always returns
nilerror, making the error return value misleading. Either return errors when validation fails or simplify the signature tofunc NewCRETransport(...) *CRETransport.- No validation is performed on required parameters (
runtimeandendpoint).🔎 Suggested validation improvements
func NewCRETransport(runtime cre.NodeRuntime, endpoint string, signer auth.Signer) (*CRETransport, error) { + if runtime == nil { + return nil, fmt.Errorf("runtime cannot be nil") + } + if endpoint == "" { + return nil, fmt.Errorf("endpoint cannot be empty") + } + return &CRETransport{ runtime: runtime, client: &http.Client{}, endpoint: endpoint, signer: signer, - chainID: "", // Will be fetched on first call if needed }, nil }
132-134: Consider accepting all 2xx HTTP status codes.The current check only accepts status code 200, which may be overly strict. Standard HTTP practice treats all 2xx codes as successful responses (e.g., 201 Created, 204 No Content).
🔎 Suggested improvement
// Check HTTP status - if httpResp.StatusCode != 200 { - return fmt.Errorf("unexpected HTTP status code: %d", httpResp.StatusCode) + if httpResp.StatusCode < 200 || httpResp.StatusCode >= 300 { + return fmt.Errorf("unexpected HTTP status code: %d", httpResp.StatusCode) }
328-358: Simplify with standard library functions.The custom case-insensitive substring search is complex and error-prone. The standard library provides simpler alternatives.
🔎 Simplified implementation using strings package
+import "strings" + // containsAny checks if a string contains any of the specified substrings (case-insensitive) func containsAny(s string, substrings []string) bool { - lowerS := s + lowerS := strings.ToLower(s) for _, substr := range substrings { - if len(substr) == 0 { - continue - } - // Simple case-insensitive substring check - for i := 0; i <= len(lowerS)-len(substr); i++ { - match := true - for j := 0; j < len(substr); j++ { - c1 := lowerS[i+j] - c2 := substr[j] - // Convert to lowercase for comparison - if c1 >= 'A' && c1 <= 'Z' { - c1 += 'a' - 'A' - } - if c2 >= 'A' && c2 <= 'Z' { - c2 += 'a' - 'A' - } - if c1 != c2 { - match = false - break - } - } - if match { - return true - } + if strings.Contains(lowerS, strings.ToLower(substr)) { + return true } } return false }
363-376: Validate chain ID is non-empty after fetch.The
fetchChainIDmethod doesn't verify that the returnedchain_idis non-empty. While the Execute method checks for this (line 249), it's better to validate at the point of fetching.🔎 Add validation
if err := t.callJSONRPC(ctx, "user.chain_info", map[string]any{}, &result); err != nil { return fmt.Errorf("failed to fetch chain ID: %w", err) } + if result.ChainID == "" { + return fmt.Errorf("chain ID is empty in response") + } + // Cache the chain ID t.chainID = result.ChainID return nil
📜 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/tnclient/transport.go (1)
Transport(38-95)
⏰ 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)
1-21: LGTM! Build constraint and imports are appropriate.The
wasip1build constraint correctly limits this implementation to the WebAssembly System Interface environment where CRE operates, and the imports include all necessary dependencies for thread-safe JSON-RPC communication.
50-62: Past review concern successfully addressed!The previous review flagged that
sync.Oncewould permanently cache chain ID fetch failures. The current implementation uses a mutex-based pattern withchainIDMuandchainIDInitialized, allowing retries on transient failures. This is the correct approach.Based on learnings from past review comments.
167-181: LGTM! Clean read-only call implementation.The method correctly constructs the JSON-RPC parameters and delegates to the common
callJSONRPChelper with proper error handling.
226-251: Double-checked locking correctly implements retry-on-failure pattern.The chain ID initialization uses double-checked locking with proper mutex management. The pattern is complex but correctly addresses the past review concern about permanent failure caching:
- Fast path with read lock (lines 229-232)
- Slow path with write lock and double-check (lines 234-246)
- Safety check for empty chain ID (lines 249-251)
This ensures transient failures don't permanently prevent chain ID fetching while maintaining thread safety.
Based on learnings from past review comments.
417-419: LGTM! Simple and correct accessor.The
Signer()method correctly returns the configured signer or nil for read-only mode.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
core/tnclient/transport_cre.go (4)
81-89: Constructor never returns an error.The function signature includes an error return but always returns
nil. Consider either adding validation (e.g., endpoint format check) or documenting why the error is preserved for future extensibility.🔎 Optional: Add basic validation or simplify
func NewCRETransport(runtime cre.NodeRuntime, endpoint string, signer auth.Signer) (*CRETransport, error) { + if endpoint == "" { + return nil, fmt.Errorf("endpoint is required") + } return &CRETransport{ runtime: runtime, client: &http.Client{}, endpoint: endpoint, signer: signer, chainID: "", // Will be fetched on first call if needed }, nil }
289-320: Initial poll delay adds unnecessary latency.The ticker-based loop waits for the full interval before the first poll. For faster feedback, consider querying immediately before starting the ticker loop.
🔎 Optional: Poll immediately before ticker loop
func (t *CRETransport) WaitTx(ctx context.Context, txHash types.Hash, interval time.Duration) (*types.TxQueryResponse, error) { ticker := time.NewTicker(interval) defer ticker.Stop() + // Helper to query transaction + queryTx := func() (*types.TxQueryResponse, error, bool) { + params := map[string]any{"tx_hash": txHash} + var result types.TxQueryResponse + if err := t.callJSONRPC(ctx, "user.tx_query", params, &result); err != nil { + if !isTransientTxError(err) { + return nil, fmt.Errorf("transaction query failed: %w", err), false + } + return nil, nil, true // transient, continue + } + if result.Height > 0 { + return &result, nil, false + } + return nil, nil, true // not finalized, continue + } + + // Immediate first check + if result, err, cont := queryTx(); !cont { + return result, err + } + for { select { case <-ctx.Done(): return nil, ctx.Err() case <-ticker.C: - // Query transaction status - params := map[string]any{ - "tx_hash": txHash, - } - - var result types.TxQueryResponse - if err := t.callJSONRPC(ctx, "user.tx_query", params, &result); err != nil { - // Distinguish between transient errors (retry-able) and permanent errors - if !isTransientTxError(err) { - // Permanent error - authentication failure, network issues, malformed request - return nil, fmt.Errorf("transaction query failed: %w", err) - } - // Transient error (tx not indexed yet) - continue polling - continue - } - - // Check if transaction is finalized (either committed or rejected) - if result.Height > 0 { - return &result, nil + if result, err, cont := queryTx(); !cont { + return result, err } } } }
413-429: Stale comment mentionssync.Once.The comment at line 414 says "This is called once via sync.Once" but the implementation now uses mutex-based lazy initialization. Update the comment to reflect the current design.
🔎 Update comment
// fetchChainID fetches and caches the chain ID from the gateway. -// This is called once via sync.Once to ensure thread-safe lazy initialization. -// Returns error if the fetch fails, which can be checked before critical operations. +// This is called lazily with mutex-based synchronization to allow retry on transient failures. +// Returns error if the fetch fails; the caller can retry on the next call. func (t *CRETransport) fetchChainID(ctx context.Context) error {
377-411: Use standard librarystringspackage instead of custom implementations.The
stringspackage is available in wasip1 and works without issues. ReplacetoLower()withstrings.ToLower(),contains()withstrings.Contains(), andindexOfSubstring()withstrings.Index(). These functions are only used for error message matching (lines wheretoLower(errMsg)andcontains(lowerMsg, pattern)appear), making stdlib equivalents the more idiomatic choice.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/tnclient/transport_cre.gocore/tnclient/transport_cre_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
core/tnclient/transport_cre.go (1)
core/tnclient/transport.go (1)
Transport(38-95)
⏰ 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 (12)
core/tnclient/transport_cre_test.go (5)
1-10: LGTM on build tag and imports.The
wasip1build constraint correctly limits these tests to the CRE/WASM environment where the implementation is available.
16-28: LGTM on constructor existence test.The test correctly verifies the function exists and acknowledges the CRE environment limitation.
30-39: LGTM on interface conformance test.Good practice to include this compile-time check in tests for documentation, even though it's already verified in the implementation file.
41-64: LGTM on option function tests.The nil parameters are acceptable for signature verification tests since the options aren't exercised beyond type checking.
68-157: Comprehensive test coverage for transient error detection.The table-driven tests thoroughly cover JSON-RPC error codes, fallback patterns, and case insensitivity. The
reasoningfield provides good documentation of expected behavior.core/tnclient/transport_cre.go (7)
1-21: LGTM on package structure and imports.Imports are well-organized with standard library first, then internal dependencies, and finally external CRE SDK packages.
50-62: LGTM on struct design and interface compliance.The struct correctly uses
sync.RWMutexwith a boolean flag for retry-capable lazy initialization, addressing the previous review concern aboutsync.Oncepermanently caching failures.
97-160: LGTM on JSON-RPC call implementation.The method correctly handles request marshaling, CRE HTTP execution, response parsing, and error propagation. The HTTP 200 check is appropriate for JSON-RPC where errors are returned in the response body.
162-181: LGTM on read-only Call implementation.The method correctly builds parameters matching the kwil-db
user.callendpoint format and delegates tocallJSONRPC.
226-251: Chain ID fetching addresses previous review concern.The double-checked locking pattern correctly allows retry on transient failures by only setting
chainIDInitialized = trueon success. This resolves the previoussync.Oncepermanent failure issue.
437-469: Retry-capable lazy initialization with timeout.The implementation correctly uses double-checked locking with a 10-second timeout, addressing the previous review concerns about permanent failure caching and indefinite hanging.
471-476: LGTM on Signer getter.Simple accessor matching the Transport interface.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
core/tnclient/transport_cre.go (3)
83-91: Constructor never returns an error.The
NewCRETransportfunction signature includes anerrorreturn, but the implementation always returnsnil. Consider simplifying the signature tofunc NewCRETransport(...) *CRETransportunless you anticipate validation or initialization errors in the future.🔎 Simplified signature
-func NewCRETransport(runtime cre.NodeRuntime, endpoint string, signer auth.Signer) (*CRETransport, error) { +func NewCRETransport(runtime cre.NodeRuntime, endpoint string, signer auth.Signer) *CRETransport { return &CRETransport{ runtime: runtime, client: &http.Client{}, endpoint: endpoint, signer: signer, chainID: "", // Will be fetched on first call if needed - }, nil + } }Note: This would be a breaking change for existing callers.
336-385: Compile regex pattern once for better performance.The regex pattern at line 347 is compiled on every call to
isTransientTxError. SinceWaitTxpolls repeatedly and calls this function each time, the compilation overhead accumulates. Compile the pattern once as a package-level variable.🔎 Proposed optimization
+// Package-level regex for extracting error codes from JSON-RPC error messages +var jsonrpcCodePattern = regexp.MustCompile(`\(code:\s*(-?\d+)\)`) + // isTransientTxError determines if an error from tx_query is transient (retry-able). // // Strategy: // 1. First, try to parse as JSON-RPC error and check error code // 2. Fall back to substring matching if not a structured JSON-RPC error // // Known transient error codes: // - -202 (ErrorTxNotFound): Transaction not yet indexed // - -32001 (ErrorTimeout): Temporary timeout // // Fragility warning: The substring fallback is brittle and may misclassify errors. // Consider adding structured error codes to the gateway API for better reliability. func isTransientTxError(err error) bool { if err == nil { return false } // Try to extract JSON-RPC error code from the error message // The error from callJSONRPC is formatted as "JSON-RPC error: <message> (code: <code>)" errMsg := err.Error() - // Use regex to extract error code from "(code: <number>)" pattern - // This handles multi-word error messages unlike fmt.Sscanf with %*s - codePattern := regexp.MustCompile(`\(code:\s*(-?\d+)\)`) - matches := codePattern.FindStringSubmatch(errMsg) + // Extract error code from "(code: <number>)" pattern + matches := jsonrpcCodePattern.FindStringSubmatch(errMsg)
387-389: Update outdated comment.Line 388 mentions
sync.Once, but the implementation now uses a mutex-based lazy initialization pattern (as seen inExecuteandChainIDmethods). Update the comment to reflect the current implementation.🔎 Suggested comment update
// fetchChainID fetches and caches the chain ID from the gateway. -// This is called once via sync.Once to ensure thread-safe lazy initialization. +// This is called lazily with mutex-based double-checked locking for thread-safe initialization. // Returns error if the fetch fails, which can be checked before critical operations.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/tnclient/transport_cre.gocore/tnclient/transport_cre_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- core/tnclient/transport_cre_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
core/tnclient/transport_cre.go (1)
core/tnclient/transport.go (1)
Transport(38-95)
⏰ 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 (2)
core/tnclient/transport_cre.go (2)
99-162: Well-structured JSON-RPC implementation.The method provides comprehensive error handling, proper validation of HTTP status and JSON-RPC version, and clear error messages. The use of CRE's promise-based HTTP client is correctly implemented.
430-433: Good timeout protection added.The 10-second timeout context prevents indefinite hanging when fetching chain metadata. This addresses the previous review concern about uncancellable contexts in
ChainID().
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/tnclient/transport_cre.go (1)
66-91: Consider validating the endpoint parameter.The constructor doesn't validate that
endpointis non-empty or a well-formed URL. While errors would surface during the first HTTP call, early validation provides clearer error messages.🔎 Optional validation enhancement
func NewCRETransport(runtime cre.NodeRuntime, endpoint string, signer auth.Signer) (*CRETransport, error) { + if endpoint == "" { + return nil, fmt.Errorf("endpoint cannot be empty") + } + // Note: URL parsing could be added but may be overly strict for CRE contexts return &CRETransport{ runtime: runtime, client: &http.Client{}, endpoint: endpoint, signer: signer, chainID: "", // Will be fetched on first call if needed }, nil }
📜 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/tnclient/transport.go (1)
Transport(38-95)
⏰ 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 (11)
core/tnclient/transport_cre.go (11)
1-23: LGTM! Clean imports and appropriate build constraint.The
wasip1build constraint correctly limits this transport to CRE/Wasm environments, and all imports are necessary for the CRE HTTP client integration.
25-64: Excellent documentation and thread-safe design!The struct documentation provides clear usage examples for CRE workflows. The implementation uses proper concurrency primitives (sync.RWMutex for chain ID caching, atomic.Uint64 for request IDs) and includes compile-time interface verification.
93-97: LGTM! Thread-safe request ID generation.The atomic operation ensures safe concurrent access without locks.
99-162: LGTM! Robust JSON-RPC implementation with comprehensive error handling.The method properly:
- Validates HTTP status (200 is correct for JSON-RPC)
- Checks JSON-RPC version and error fields
- Conditionally unmarshals results
- Provides descriptive error messages at each stage
164-183: LGTM! Clean read-only action implementation.The method correctly maps to kwil-db's
user.callendpoint with appropriate parameter structure.
185-282: Excellent implementation with proper chain ID handling!The Execute method correctly:
- Validates signer presence upfront
- Encodes inputs using kwil-db types
- Uses double-checked locking for chain ID initialization
- Only marks chain ID as initialized after successful fetch with validation (addresses past review feedback)
- Signs and broadcasts the transaction
The chain ID validation fix from previous reviews has been properly implemented.
284-319: LGTM! Robust transaction polling with proper error classification.The method correctly:
- Uses ticker with deferred cleanup
- Handles context cancellation
- Distinguishes transient (retry-able) from permanent errors
- Finalizes on Height > 0
321-382: Excellent fix for error code extraction!The regex-based approach correctly addresses the previous Sscanf issue with multi-word error messages. The pattern
\(code:\s*(-?\d+)\)reliably extracts error codes from the JSON-RPC error format, and the fallback substring matching is appropriately documented as fragile.The fix properly handles:
- Multi-word error messages (previous Sscanf with
%*sfailed here)- Optional whitespace
- Negative error codes
384-405: Perfect! Chain ID validation happens before caching.The validation at lines 398-400 ensures that empty chain IDs are rejected before caching, which prevents the initialized flag from being set on failures. This properly addresses the previous review feedback about validating before marking as initialized.
407-445: Excellent timeout protection added!The method now creates a 10-second timeout context (lines 434-435) to prevent indefinite hangs, properly addressing the previous review feedback about using uncancellable
context.Background(). The double-checked locking pattern and retry-on-failure behavior (not setting initialized flag on error) are both correct.
447-452: LGTM! Simple and well-documented accessor.
1 participant
resolves: https://github.com/truflation/website/issues/3055
Summary by CodeRabbit
New Features
Chores
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.