fix(erpc:PLA-1613): isolate vendor cache keys#97
Conversation
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
| return fmt.Sprintf("%s|provider=%s|key=%s", vendorName, providerID, hex.EncodeToString(h.Sum(nil))[:24]) | ||
| } | ||
|
|
||
| type cacheHash interface { |
There was a problem hiding this comment.
The 'cacheHash' interface duplicates io.Writer; use io.Writer instead to avoid an unnecessary local type and simplify the code.
Details
✨ AI Reasoning
A new small helper interface is introduced solely to expose a Write([]byte) method. The standard library already provides io.Writer with the exact signature. Using io.Writer would reduce indirection and avoid defining an unnecessary local type, improving readability without changing behavior.
🔧 How do I fix it?
Rewrite the snippet in the simpler, behavior-equivalent form: return a boolean expression directly instead of if cond return true else return false, avoid using lists when they are guaranteed to contain one element, etc.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
There was a problem hiding this comment.
Pull request overview
Scopes vendor remote-catalog cache keys by provider id and vendor-specific inputs (and hashes secret-bearing parts) so multiple providers can safely share the same vendor account credentials without RemoteDataCache snapshot collisions.
Changes:
- Introduces a shared cache-key helper (
vendorCapabilityCacheKey) that mixes vendor name + provider id + hashed parts (withsecretCachePart). - Updates QuickNode, Chainstack, and Repository vendors to use provider-scoped cache keys (and adjusts tests accordingly).
- Updates provider docs to reflect the new QuickNode tag-group behavior (with examples).
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| thirdparty/vendor_cache_key.go | Adds helper utilities for provider-scoped, secret-safe remote cache keys. |
| thirdparty/vendor_cache_key_test.go | Adds focused tests ensuring provider isolation and secret redaction in keys. |
| thirdparty/repository.go | Scopes repository remote cache entries by provider id + repository URL (hashed into key). |
| thirdparty/quicknode.go | Scopes QuickNode endpoint snapshots by provider id + apiKey (hashed) + tag filters. |
| thirdparty/quicknode_test.go | Updates snapshot keys to match new QuickNode cache-key scheme; adds reuse test. |
| thirdparty/provider.go | Injects provider id into vendor settings to enable provider-scoped cache keys. |
| thirdparty/provider_test.go | Verifies provider id injection does not mutate original settings; adds template test. |
| thirdparty/chainstack.go | Scopes Chainstack node snapshots by provider id + apiKey (hashed) + filters. |
| thirdparty/chainstack_test.go | Updates cache-key test to assert provider scoping and no raw apiKey leakage. |
| docs/pages/config/projects/providers.mdx | Updates QuickNode guidance and examples for provider-id + tag-based isolation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| apiKey: \${QN_ARCHIVE_KEY} | ||
| apiKey: \${QN_API_KEY} | ||
| tagLabels: archive | ||
| # fast tip-of-chain endpoints on a separate key |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b04d2e4bf2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| key := name + "\x00" + value | ||
| if cached, ok := secretCachePartIDs.Load(key); ok { | ||
| return name + "=secret:" + cached.(string) | ||
| } |
| return name + "=secret:" + hex.EncodeToString(mac.Sum(nil))[:24] | ||
| } | ||
|
|
||
| func writeSecretCachePart(mac hashWriter, name, value string) { |
There was a problem hiding this comment.
writeSecretCachePart issues multiple small writes and temporary byte conversions; combine into a single write (single buffer or formatted write) to simplify and reduce allocations.
Details
✨ AI Reasoning
The helper writeSecretCachePart writes the name, separator, length and value via four separate mac.Write calls with temporary byte conversions. This is more verbose than necessary and creates repeated allocations for small byte slices. Building a single byte slice (or using fmt.Fprintf with a bytes.Buffer) and writing once would be simpler and clearer while preserving semantics.
🔧 How do I fix it?
Rewrite the snippet in the simpler, behavior-equivalent form: return a boolean expression directly instead of if cond return true else return false, avoid using lists when they are guaranteed to contain one element, etc.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
thirdparty/provider.go:165
applyUpstreamIDTemplatenow substitutes<EVM_CHAIN_ID>with the literal stringN/Afor non-evm:networks. This changes upstream ID generation behavior (previously the placeholder would typically be removed/empty), which can be a breaking change for deployments relying on stable IDs/metrics naming, and it’s not mentioned in the PR description (which focuses on vendor cache keys). Consider either (a) reverting to the prior empty-string behavior for backward compatibility, or (b) explicitly calling out this behavior change in the PR description/release notes so users can opt in by updating templates.
// If network is in "evm:<someChainId>" format, then <EVM_CHAIN_ID> = <someChainId>.
// Otherwise, we replace that placeholder with N/A.
if strings.HasPrefix(networkId, "evm:") {
evmChainId := strings.TrimPrefix(networkId, "evm:")
result = strings.ReplaceAll(result, "<EVM_CHAIN_ID>", evmChainId)
} else {
result = strings.ReplaceAll(result, "<EVM_CHAIN_ID>", "N/A")
}
Summary
Linear: https://linear.app/morpho-labs/issue/PLA-1613/fix-erpc-provider-mapping-vendor-cache-and-repository-method
Validation
go test ./thirdparty ./common -count=1Notes
go test ./...was attempted locally but Docker-backed tests fail on this host withrootless Docker not found, failed to create Docker provider.