feat: configure cache on CRE settings#169
Conversation
📝 WalkthroughWalkthroughAdds optional HTTP caching to CRE transport: new CREHTTPCacheConfig, cache-aware constructor and option, per-call CacheSettings and cache-key derivation (SHA-256 when caching), wiring CacheSettings into CRE HTTP requests, tests, and docs updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CRETransport
participant Signer
participant CRENode as CRE NodeRuntime
participant HTTPCache as HTTP Cache/Store
Client->>CRETransport: doJSONRPC(method, params)
CRETransport->>CRETransport: paramsJSON := marshal(params)
CRETransport->>CRETransport: cs := cacheSettingsForJSONRPC(method, paramsJSON)
alt cs.Store == true
CRETransport->>CRETransport: reqID := nextReqID(method, paramsJSON) -- SHA256 deterministic
CRETransport->>HTTPCache: check cache for reqID
HTTPCache-->>CRETransport: cached response / miss
else
CRETransport->>CRETransport: reqID := nextMonotonicReqID() -- non-cache path
end
CRETransport->>Signer: Sign request (includes reqID/payload)
Signer-->>CRETransport: signature
CRETransport->>CRENode: HTTP POST /rpc/v1 with body + CacheSettings
CRENode-->>CRETransport: HTTP response
alt cacheable and store enabled
CRETransport->>HTTPCache: store response with reqID and max-age
end
CRETransport-->>Client: parsed JSON-RPC response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)core/tnclient/options_cre.go (2)
🔇 Additional comments (1)
✏️ Tip: You can disable this entire section by setting 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 |
![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
🤖 Fix all issues with AI agents
In `@core/tnclient/transport_cre.go`:
- Around line 187-197: The comment in the cacheSettingsForJSONRPC method
contains an extraneous citation artifact
":contentReference[oaicite:2]{index=2}"; remove that artifact (or replace it
with a proper documentation link or plain explanatory text) so the comment reads
cleanly; locate the cacheSettingsForJSONRPC method on CRETransport and update
the inline comment above the return to omit the citation marker.
🧹 Nitpick comments (3)
docs/CRE_INTEGRATION.md (1)
171-199: Documentation is informative but could benefit from SDK-specific usage example.The section explains the CRE caching concept well with the raw CRE HTTP example. However, since this PR introduces
WithCRETransportAndSignerWithHTTPCache, consider adding an SDK-specific example showing how users can configure caching via the SDK:// Configure CRE transport with custom cache settings cacheCfg := &tnclient.CREHTTPCacheConfig{ Store: ptr(true), MaxAgeSeconds: ptr(int64(120)), } client, err := tnclient.NewClient(ctx, endpoint, tnclient.WithCRETransportAndSignerWithHTTPCache(nodeRuntime, endpoint, signer, cacheCfg), )This would help users understand how the SDK abstracts the raw CRE caching configuration.
core/tnclient/options_cre.go (1)
111-118: Error from transport creation is silently ignored.The error from
NewCRETransportWithHTTPCacheis discarded. If transport creation fails,c.transportremainsnil, which could cause nil pointer dereferences when the client attempts to use it.This follows the existing pattern in
WithCRETransportandWithCRETransportAndSigner, but consider whether theOptionpattern should be refactored to surface initialization errors. At minimum, consider adding a comment explaining this design decision.Alternative: Capture error for later surfacing
One approach used in some Go codebases is to store the first error and check it in
NewClient:func WithCRETransportAndSignerWithHTTPCache(runtime cre.NodeRuntime, endpoint string, signer auth.Signer, cacheCfg *CREHTTPCacheConfig) Option { return func(c *Client) { c.signer = signer var err error c.transport, err = NewCRETransportWithHTTPCache(runtime, endpoint, signer, cacheCfg) if err != nil && c.initErr == nil { c.initErr = fmt.Errorf("CRE transport initialization failed: %w", err) } } }core/tnclient/transport_cre_test.go (1)
454-459: Consider usingstrings.HasPrefixfrom the standard library.The custom
stringsHasPrefixfunction duplicatesstrings.HasPrefix. Sincestringsis already commonly imported in test files, using the stdlib version would be more idiomatic.Suggested change in test usage
-assert.True(t, stringsHasPrefix(id1, "tn:"), "expected deterministic id to have tn: prefix") +assert.True(t, strings.HasPrefix(id1, "tn:"), "expected deterministic id to have tn: prefix")And remove the custom helper function.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/tnclient/options_cre.gocore/tnclient/transport_cre.gocore/tnclient/transport_cre_test.godocs/CRE_INTEGRATION.md
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-12-24T14:19:01.755Z
Learnt from: MicBun
Repo: trufnetwork/sdk-go PR: 168
File: examples/truf-cre-demo/truf-read-workflow/config.json:1-5
Timestamp: 2025-12-24T14:19:01.755Z
Learning: Chainlink Runtime Environment (CRE) has a limitation of 5 HTTP requests per workflow, which is why the truf-cre-demo example in trufnetwork/sdk-go is split into 3 separate workflows (write, read, cleanup).
Applied to files:
docs/CRE_INTEGRATION.md
📚 Learning: 2025-12-24T14:19:01.755Z
Learnt from: MicBun
Repo: trufnetwork/sdk-go PR: 168
File: examples/truf-cre-demo/truf-read-workflow/config.json:1-5
Timestamp: 2025-12-24T14:19:01.755Z
Learning: In the trufnetwork/sdk-go repository's CRE examples, each workflow requires its own config.json file following the CRE CLI (`cre init`) format requirements, which necessitates config file duplication across workflows.
Applied to files:
docs/CRE_INTEGRATION.md
📚 Learning: 2025-12-24T14:19:01.755Z
Learnt from: MicBun
Repo: trufnetwork/sdk-go PR: 168
File: examples/truf-cre-demo/truf-read-workflow/config.json:1-5
Timestamp: 2025-12-24T14:19:01.755Z
Learning: In the trufnetwork/sdk-go repository's examples/truf-cre-demo directory, the private key "0000000000000000000000000000000000000000000000000000000000000001" is a dummy/placeholder key used for demonstration purposes in the CRE workflow examples.
Applied to files:
docs/CRE_INTEGRATION.md
📚 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:
docs/CRE_INTEGRATION.md
🧬 Code graph analysis (2)
core/tnclient/options_cre.go (2)
core/tnclient/transport_cre.go (2)
CREHTTPCacheConfig(179-185)NewCRETransportWithHTTPCache(116-123)core/tnclient/client.go (1)
Option(29-29)
core/tnclient/transport_cre_test.go (1)
core/tnclient/transport_cre.go (3)
NewCRETransport(97-114)CREHTTPCacheConfig(179-185)CRETransport(59-75)
🔇 Additional comments (16)
docs/CRE_INTEGRATION.md (1)
219-219: LGTM!Good addition of the CRE HTTP documentation link for reference.
core/tnclient/transport_cre.go (9)
8-9: LGTM!Imports for SHA-256 hashing and hex encoding are appropriate for the cache key generation feature.
73-74: LGTM!The cache configuration fields follow the same pattern as other initialization-time fields like
endpointandsigner. SinceApplyHTTPCacheConfigis intended to be called during client setup (before concurrent use), no mutex protection is needed.
106-113: Caching enabled by default - verify this is the intended behavior.The constructor now enables HTTP caching by default (
Store: true,MaxAge: 60s). This is a behavioral change that affects all existing CRE transport users.While the PR description suggests this is intentional for preventing duplicate requests across DON nodes, please verify this default won't cause issues for users who rely on fresh responses (e.g., polling for transaction status).
Note:
WaitTxusesuser.tx_querywhich will also be cached. This could cause delayed transaction confirmation detection if the cached response still showsHeight: 0.
116-123: LGTM!Clean constructor that composes
NewCRETransportwithApplyHTTPCacheConfig.
125-147: LGTM!Well-designed configuration application with:
- Nil-safe handling (noop for nil config)
- Proper clamping of negative values to zero
- Enforcement of CRE's 10-minute max cache age limit
- Pointer semantics allowing partial config updates
149-166: LGTM!The cache-aware request ID generation is well-designed:
- Uses SHA-256 hash of method + params for deterministic IDs across DON nodes
- Null byte separator prevents method/params boundary collisions
- 8-byte hash prefix provides sufficient uniqueness for cache keys
- Preserves original monotonic counter behavior when caching is disabled
234-265: LGTM!The
doJSONRPCmethod correctly integrates cache settings:
- Computes
paramsJSONfirst (needed for both the request body and cache key)- Gets
cacheSettingsusing the method and params- Generates deterministic
reqIDusing the same inputs- Attaches
CacheSettingsto the HTTP request
538-567: LGTM!The
executeOncemethod properly constructs theparamsJSONfor cache key generation before attachingCacheSettingsto the HTTP request. The additional marshaling step is necessary since this method manually constructs the JSON-RPC request.
868-889: LGTM!The
doJSONRPCWithResponsemethod follows the same pattern asdoJSONRPCfor cache settings integration, ensuring consistency across all RPC call paths.core/tnclient/transport_cre_test.go (6)
7-16: LGTM!Imports are appropriate for the AST-based static analysis tests and extended assertions.
27-45: LGTM!Good test coverage for:
- Endpoint normalization (
/rpc/v1suffix)- Default cache settings (
Store: true,MaxAge: 60s)cacheSettingsForJSONRPCproducing valid settings
172-237: LGTM!Excellent test coverage for
ApplyHTTPCacheConfig:
- Nil config handling (noop)
- Override application
- Negative value clamping
- Maximum value clamping to CRE limit
The tests verify both the internal transport state and the
cacheSettingsForJSONRPCoutput, ensuring the configuration flows through correctly.
239-262: LGTM!Good coverage verifying that all JSON-RPC methods receive the default cache settings uniformly.
264-298: LGTM!Thorough testing of
nextReqIDbehavior:
- Deterministic IDs when caching is active (idempotent for same inputs)
- Different IDs for different params or methods
- Correct format (
tn:prefix + 16 hex chars)- Monotonic sequence when caching is disabled (backward compatibility)
300-338: LGTM - Creative AST-based verification!This is a clever approach to verify that cache settings are properly wired into HTTP requests without requiring a CRE runtime. The test ensures implementation correctness at the source code level.
Note: This test is inherently coupled to the code structure and will need updates if the implementation is refactored (e.g., function renamed, variable names changed).
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Time Submission Status
|
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/tnclient/transport_cre.go (1)
868-889: Authentication requests should not be cached.Both
kgw.authn_param(fetched nonce) andkgw.authn(signed auth request) are cached by default with a 60-second TTL. This creates several problems:
- Nonce reuse: A cached nonce could be reused across multiple authentication attempts within the 60-second window, causing duplicate or failed auth submissions.
- Stale auth cookies: Cached
kgw.authnresponses may return expired or invalid Set-Cookie headers if the auth credentials change.- No per-request override: The current implementation does not support disabling caching on a per-request basis. Cache settings are configured only at transport initialization (
NewCRETransportWithHTTPCache/ApplyHTTPCacheConfig) and apply globally.The
cacheSettingsForJSONRPCmethod acceptsmethodandparamsJSONparameters but does not use them; future refactoring could add conditional logic to exclude authentication methods from caching, but this is not implemented.Recommendation: Either (1) disable caching globally for authentication requests by excluding them from the cached HTTP calls, or (2) implement per-request cache override logic in
cacheSettingsForJSONRPCto detect and skip caching forkgw.authnandkgw.authn_parammethods.
♻️ Duplicate comments (1)
core/tnclient/transport_cre.go (1)
187-197: Past review comment has been addressed.The citation artifact (
contentReference[oaicite:2]{index=2}) from the previous review has been removed. The comment now reads cleanly.
🧹 Nitpick comments (2)
core/tnclient/options_cre.go (1)
111-118: Consider documenting the function and handling the error.The function lacks a documentation comment unlike
WithCRETransportandWithCRETransportAndSigner. Additionally, the error fromNewCRETransportWithHTTPCacheis silently discarded. While this is consistent with the existing options in this file, it means a misconfiguration could result in a nil transport with no indication of failure.Consider at minimum adding a doc comment, and optionally logging or returning the error:
Suggested documentation
+// WithCRETransportAndSignerWithHTTPCache is a convenience function that combines +// WithSigner and WithCRETransport with HTTP cache configuration. +// +// This ensures the signer is set before creating the CRE transport with the +// specified cache settings, which is necessary for write operations that benefit +// from CRE's request de-duplication. func WithCRETransportAndSignerWithHTTPCache(runtime cre.NodeRuntime, endpoint string, signer auth.Signer, cacheCfg *CREHTTPCacheConfig) Option {core/tnclient/transport_cre_test.go (1)
473-478: Consider usingstrings.HasPrefixfrom the standard library.The
stringsHasPrefixhelper function duplicates the functionality ofstrings.HasPrefixfrom the standard library. Sincestringsis a standard package and would have no additional cost, you could simplify by using it directly.Suggested change
-func stringsHasPrefix(s, prefix string) bool { - if len(prefix) > len(s) { - return false - } - return s[:len(prefix)] == prefix -}And update line 293:
- assert.True(t, stringsHasPrefix(id1, "tn:"), "expected deterministic id to have tn: prefix") + assert.True(t, strings.HasPrefix(id1, "tn:"), "expected deterministic id to have tn: prefix")
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/tnclient/options_cre.gocore/tnclient/transport_cre.gocore/tnclient/transport_cre_test.godocs/CRE_INTEGRATION.md
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-12-24T14:19:01.755Z
Learnt from: MicBun
Repo: trufnetwork/sdk-go PR: 168
File: examples/truf-cre-demo/truf-read-workflow/config.json:1-5
Timestamp: 2025-12-24T14:19:01.755Z
Learning: Chainlink Runtime Environment (CRE) has a limitation of 5 HTTP requests per workflow, which is why the truf-cre-demo example in trufnetwork/sdk-go is split into 3 separate workflows (write, read, cleanup).
Applied to files:
docs/CRE_INTEGRATION.md
📚 Learning: 2025-12-24T14:19:01.755Z
Learnt from: MicBun
Repo: trufnetwork/sdk-go PR: 168
File: examples/truf-cre-demo/truf-read-workflow/config.json:1-5
Timestamp: 2025-12-24T14:19:01.755Z
Learning: In the trufnetwork/sdk-go repository's CRE examples, each workflow requires its own config.json file following the CRE CLI (`cre init`) format requirements, which necessitates config file duplication across workflows.
Applied to files:
docs/CRE_INTEGRATION.md
📚 Learning: 2025-12-24T14:19:01.755Z
Learnt from: MicBun
Repo: trufnetwork/sdk-go PR: 168
File: examples/truf-cre-demo/truf-read-workflow/config.json:1-5
Timestamp: 2025-12-24T14:19:01.755Z
Learning: In the trufnetwork/sdk-go repository's examples/truf-cre-demo directory, the private key "0000000000000000000000000000000000000000000000000000000000000001" is a dummy/placeholder key used for demonstration purposes in the CRE workflow examples.
Applied to files:
docs/CRE_INTEGRATION.md
📚 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:
docs/CRE_INTEGRATION.md
🧬 Code graph analysis (1)
core/tnclient/options_cre.go (2)
core/tnclient/transport_cre.go (2)
CREHTTPCacheConfig(179-185)NewCRETransportWithHTTPCache(116-123)core/tnclient/client.go (1)
Option(29-29)
⏰ 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 (9)
docs/CRE_INTEGRATION.md (2)
171-198: LGTM! Clear documentation for CRE HTTP caching.The documentation clearly explains the purpose of CRE HTTP caching for non-idempotent operations and provides a practical code example. The guidance about when to use caching (POST/PUT/PATCH/DELETE operations) is helpful for developers.
219-219: Good addition of the CRE HTTP documentation link.The external reference to the official CRE HTTP documentation is a useful resource for developers seeking more details.
core/tnclient/transport_cre_test.go (3)
319-357: Clever use of AST parsing for verifying internal wiring.The static analysis approach to verify that
CacheSettingsis properly wired into HTTP requests is a good solution given the CRE runtime limitations. This ensures the caching logic is correctly integrated without needing to execute the actual CRE HTTP client.
283-317: Good test coverage for both caching modes.The tests appropriately verify both the deterministic ID generation when caching is active (using SHA-256 hash) and the monotonic sequence when caching is disabled. The assertions for ID format (prefix and length) are correct.
191-256: Comprehensive tests for ApplyHTTPCacheConfig edge cases.The tests cover nil config (no-op), explicit overrides, negative value clamping, and maximum value clamping. This ensures the configuration handling is robust.
core/tnclient/transport_cre.go (4)
149-166: Good cache-aware request ID implementation.The logic correctly generates deterministic IDs (using SHA-256 of method + params) when caching is active, which ensures CRE can deduplicate requests across nodes. The fallback to monotonic IDs when caching is disabled maintains backward compatibility. The null byte separator between method and params prevents collision between e.g. method "a" + params "bc" and method "ab" + params "c".
125-147: Robust configuration handling with proper bounds clamping.The
ApplyHTTPCacheConfigmethod correctly handles nil configs, applies overrides only when values are provided, and clamps values to valid ranges (negative → 0, excessive → max). This defensive approach prevents invalid cache configurations.
177-185: Well-documented configuration type.The
CREHTTPCacheConfigstruct is clearly documented with default behavior and clamping rules. The use of pointers for optional fields allows distinguishing "not set" from "explicitly set to false/zero".
538-546: Verify the necessity of re-marshaling params for broadcast cache key.The
paramsJSONis constructed by re-marshaling the transaction JSON into abroadcastParamsstruct. This works correctly, but note that the cache key will be derived from{"tx":<txJSON>}rather than justtxJSON. This is functionally correct since the same transaction will produce the same cache key, but it's worth documenting this behavior if the cache key format matters for debugging.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
@MicBun the PR is blocked on cost submission for holdex which I am not part of. I would rather not give it auth to act on my github behalf. Anyway we can disable that check to be able to merge the PR? |
2 participants
CRE provides cache settings. The initial implementation of the CRE transport did not allow this to be set.
Apart from the setting of the CRE cache settings there is a change to reqId creation. If there is any WaitForTx or any other issue like a retry / 401 the Id would move on rendering the caching invalid. The functionality of id generation is only changed for when cache settings are set.
How Has This Been Tested?
Structural go unit test extended and pass
CRE workflow truflation-sdk-test deployed to CRE mainnet.
Workflow references package whose location was overridden so that the local repo was referenced rather than the github release location.
Calls work fine.
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.