fix(portfolio): route account fetches around metadata-stripping RPC nodes - #66
Conversation
…odes Some Hive nodes serve accounts with posting_json_metadata empty while every other field is correct. That is a well-formed get_accounts array, so the shape validation passes and the latency EWMA keeps such a node ranked first. Portfolio token visibility is derived entirely from that field, so the engine allowlist came back empty and BuildEngineLayer dropped every token; the chain layer, reading the same metadata, dropped every wallet. Balances still rendered from account fields, so the response was a normal 200 with no error and nothing logged, and accounts with enabled tokens showed none of them. Two of the pool's fastest nodes behave this way, so they won the ranking and the layers were empty for essentially every request. - Drop those two nodes from the pool. - Add a soft result preference to Call: unlike validateResult it is not a health signal, so the node is neither retried nor marked unhealthy; we keep its answer and move on. GetAccounts uses it to prefer a node that serves metadata. - Bound the probing to two nodes. Roughly an eighth of active accounts genuinely carry no metadata and no node can satisfy the preference for them, so sweeping the pool would multiply RPC load on a common case to route around a rare one. If no node satisfies it, the first well-formed answer is returned, never worse than before.
|
Warning Review limit reached
Next review available in: 43 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesHive metadata failover
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GetAccounts
participant HiveRpcClient
participant HiveNode
GetAccounts->>HiveRpcClient: Request account data with metadata preference
HiveRpcClient->>HiveNode: Call get_accounts
HiveNode-->>HiveRpcClient: Valid account response
HiveRpcClient->>HiveRpcClient: Check posting_json_metadata
HiveRpcClient->>HiveNode: Probe another node when metadata is absent
HiveNode-->>HiveRpcClient: Preferred response or valid fallback
HiveRpcClient-->>GetAccounts: Return account response
Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dotnet/EcencyApi.Tests/HiveRpcFailoverTests.cs`:
- Around line 246-261: The NoNodeServesMetadata_StillReturnsTheAccount test does
not verify that failover returns the first valid fallback. Add an assertion that
accounts[0]["port"] matches a.Url, while preserving the existing non-null and
name assertions; keep the RPC failover behavior covered by this test.
In `@dotnet/EcencyApi/Infrastructure/HiveRpcClient.cs`:
- Around line 107-122: Update the GetAccounts preference-failure path around
preferResult and the node-selection state to record a method-specific negative
capability or ordering penalty for the responding node, without changing generic
health success tracking used by other RPC methods. Apply that state during
subsequent GetAccounts node selection so the metadata-stripping node is skipped
or deprioritized on the next call, and add a two-call test verifying it receives
no second account request.
- Around line 264-292: The get_accounts result validation currently accepts
scalar array entries and can prefer malformed responses. Update validateResult
and the HasAnyAccountMetadata path to require every array element to be a
JsonObject or JSON null, while preserving all-null arrays as valid
unknown-account responses; reject arrays containing scalars so failover
proceeds. Add a test covering a scalar-array response and verifying failover.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0d8ae4dc-23ad-4048-b3cb-88c738747437
📒 Files selected for processing (2)
dotnet/EcencyApi.Tests/HiveRpcFailoverTests.csdotnet/EcencyApi/Infrastructure/HiveRpcClient.cs
![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
Preserve the cached result when a preference probe gets an RPC error
When the first node returns a valid but unpreferred account and the next node responds with a JSON-RPC error, this unconditional rethrow discards unpreferred and makes GetAccounts fail. Before this change the first response would have succeeded, contradicting the new soft preference's stated guarantee that callers are never worse off; once haveUnpreferred is true, a probe-specific RPC error should fall back to that cached valid response rather than turning the optional probe into a request failure.
ℹ️ 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".
Greptile SummaryThis PR makes account retrieval prefer well-formed RPC responses containing posting metadata while retaining the first valid response as a fallback.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| dotnet/EcencyApi/Infrastructure/HiveRpcClient.cs | Adds soft metadata-aware failover, stronger account-result validation, bounded preference probing, and removes known metadata-stripping nodes. |
| dotnet/EcencyApi.Tests/HiveRpcFailoverTests.cs | Expands failover tests to cover metadata preference, fallback preservation, malformed entries, RPC probe errors, and probe bounds. |
Sequence Diagram
sequenceDiagram
participant Caller
participant Client as HiveRpcClient
participant Node1 as Preferred RPC order: Node 1
participant Node2 as Preferred RPC order: Node 2
Caller->>Client: GetAccounts(names)
Client->>Node1: condenser_api.get_accounts
Node1-->>Client: Well-formed result
alt Metadata is present
Client-->>Caller: Return result
else Metadata is absent
Client->>Node2: Probe alternative node
alt Node 2 returns metadata
Node2-->>Client: Preferred result
Client-->>Caller: Return Node 2 result
else Preference remains unsatisfied or probe errors
Node2-->>Client: Unpreferred result or RPC error
Client-->>Caller: Return first well-formed result
end
end
Reviews (2): Last reviewed commit: "review: keep the cached answer when a pr..." | Re-trigger Greptile
…ount arrays - An RPC-level error from a node consulted only to improve on an answer we already hold belongs to the optional probe, not to the caller's request. Rethrowing it failed a call that would have succeeded without the preference, contradicting the "never worse off" guarantee. - get_accounts validated only "is an array", so entries like ["invalid"] passed and then read as empty downstream — silently blanking portfolio token visibility exactly like a metadata-stripping node. Require account objects or JSON null (unknown account) and fail over otherwise. - Pin the first-well-formed-answer contract in the no-metadata test.
|
Triaged all four findings. Three were real and are fixed in 0bd4dab; one is declined with reasoning. Fixed — Codex, "preserve the cached result when a preference probe gets an RPC error" (P2). Correct, and it was a regression this PR introduced: Fixed — CodeRabbit, "reject scalar entries in Fixed — CodeRabbit, "assert the first valid fallback is returned" (Minor). Fair; the test passed whether the first or last unpreferred answer came back. Now asserts the port of the first node. Both new behaviours are covered by tests that were confirmed to fail without the fix and pass with it. Suite: 100 passed. Declined — CodeRabbit, "deprioritize metadata-stripping nodes for later
Happy to revisit if a pool node starts stripping metadata. |
|
@coderabbitai review |
|
|
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Fixes #64.
Problem
/wallet-api/portfolio-v2withonlyEnabled: true(what the web and mobile clients send) returned zero engine tokens and zero chain wallets for accounts that had explicitly enabled them — a normal200, correct Hive balances, nothing logged.Token visibility is derived from the account's
posting_json_metadata. Two nodes in the pool serve accounts with that field empty while every other field is correct:posting_json_metadatatechcoderx.comhiveapi.actifit.ioapi.hive.blog,api.deathwing.me,rpc.mahdiyari.info,api.openhive.networkThat is a well-formed
get_accountsarray, so the shape validation passes and the node is recorded healthy. Both are among the fastest in the pool, so the latency EWMA kept them ranked first and the layers were empty for essentially every request.ExtractEnabledEngineTokenSymbolsreturned an empty set,BuildEngineLayer(onlyEnabled: true, …)dropped every token, andExtractExternalWalletsdropped every chain wallet. The Hive layer still rendered from account fields, which is why the failure looked like "my engine balances vanished" rather than an outage.Same class as the malformed-200 case already described in
CLAUDE.md: a node that answers successfully but uselessly scores as healthy and stays ranked first.Change
HiveClients.Default, so correctness here does not depend on the runtime fallback firing.preferResulttoHiveRpcClient.Call. UnlikevalidateResultit is not a health signal — the node is fine for other calls — so it is neither retried nor marked unhealthy; its answer is kept and we move to the next node.GetAccountsuses it to prefer a node that actually serves metadata.Verification
Built and run locally against the real endpoint,
onlyEnabled: true:Production returned zero engine tokens on 30/30 consecutive requests for these accounts before the change.
Tests
dotnet test: 98 passed. Three new cases inHiveRpcFailoverTests:The stub node now serves a realistic account payload including
posting_json_metadata, with aServesMetadatatoggle.Summary by CodeRabbit