Forking: bank balances local-first with remote fallback and graceful NotFound

[tiljrd]

Full Forking Bank Keeper Implementation

Summary

Implements a complete forking bank keeper under x/bloctopus that replaces THORChain's standard bank keeper when forking is enabled. The keeper provides transparent access to mainnet balance data while maintaining local-first behavior and materializing remote state on writes.

Key Features:

• Read Path: Local storage first; if account/denom not found, queries mainnet gRPC (grpc.thor.pfc.zone) and returns merged balances with local precedence
• Write Path: Before mutations, materializes remote baseline for accounts not present locally by minting to thorchain module then sending to target account
• Full Interface Compliance: Implements complete bankkeeper.Keeper interface so all modules (staking/mint/wasm/authz/denom/thorchain) use it transparently
• Feature-Flagged: Only active when --fork.grpc and --fork.chain-id flags are provided

Architecture:

• ForkingBankKeeper embeds bankkeeper.BaseKeeper and adds remote gRPC client
• Overrides balance read methods (GetBalance, GetAllBalances, IterateAccountBalances) for remote fallback
• Overrides write methods to call ensureMaterialized() before base operations
• All other methods forward directly to base keeper

Review & Testing Checklist for Human

⚠️ HIGH RISK - 5 Critical Items

• End-to-end forking behavior verification: Start node with --fork.grpc=grpc.thor.pfc.zone:443 --fork.chain-id=thorchain-1, query balance of mainnet-only address via /cosmos/bank/v1beta1/balances/{addr}, send coins to that address, re-query to confirm remote+local merge
• Keeper interface completeness audit: Verify all ~50+ forwarded methods have correct signatures by checking against latest Cosmos SDK bank keeper interface - any mismatch will cause runtime panics
• Materialization invariant testing: Confirm that minting remote balances to thorchain module doesn't break supply tracking, module balance limits, or other chain invariants during write operations
• Remote failure resilience: Test behavior when grpc.thor.pfc.zone is unreachable - should gracefully fall back to local-only without breaking core functionality
• Write operation coverage: Test all write paths (SendCoins, module transfers, delegation, etc.) with accounts that exist only on mainnet to ensure materialization works correctly

Notes

• Materialization Strategy: Uses mint-to-module + send-to-account pattern which could have supply tracking implications
• Error Handling: Remote client returns (nil, nil) on errors, masking potential issues but preventing crashes
• No Visible Tests: This core functionality change lacks accompanying test coverage in the diff
• Connection Lifecycle: gRPC connections created but no explicit cleanup visible

Link to Devin run: https://app.devin.ai/sessions/005c15c5a6a54dab8494af7a0e7f5ad5
Requested by: Til Jordan (@tiljrd)

[devin-ai-integration]

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

• Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
• Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

• Disable automatic comment and CI monitoring

[qodo-code-review]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Possible Issue The relaxed NotFound handling in fetchBalanceData will return nil data and nil error, which may be indistinguishable from "no remote attempt" or "empty but valid". Confirm downstream callers properly differentiate between "remote empty" vs "codec/marshal empty" and that caching layers won't treat nil as a cacheable empty response that suppresses future remote retries. resp, err := c.queryClient.Balances(ctx, req) if err != nil { if isNotFoundErr(err) { return nil, nil } return nil, fmt.Errorf("gRPC balances query failed: %w", err) } return c.codec.Marshal(resp) } Fragile Parsing extractAddressFromKey duplicates near-identical blocks for 'account/', 'balances/', 'balance/' and looser 'account'/'balances' matches. Risk of false positives and maintenance burden; consider centralizing parsing with a unified helper and validating bech32 format to avoid extracting non-address substrings. func (c *remoteClient) extractAddressFromKey(key string) string { lower := strings.ToLower(key) if idx := strings.Index(lower, "account/"); idx != -1 { rest := strings.Trim(key[idx+len("account/"):], "/") if rest != "" { if i := strings.Index(rest, "/"); i != -1 { return rest[:i] } return rest } } if idx := strings.Index(lower, "balances/"); idx != -1 { rest := strings.Trim(key[idx+len("balances/"):], "/") if rest != "" { if i := strings.Index(rest, "/"); i != -1 { return rest[:i] } return rest } } if idx := strings.Index(lower, "balance/"); idx != -1 { rest := strings.Trim(key[idx+len("balance/"):], "/") if rest != "" { if i := strings.Index(rest, "/"); i != -1 { return rest[:i] } return rest } } if idx := strings.Index(lower, "account"); idx != -1 { rest := strings.TrimSpace(key[idx+len("account"):]) rest = strings.TrimLeft(rest, "/") if rest != "" { if i := strings.Index(rest, "/"); i != -1 { return rest[:i] } return rest } } if idx := strings.Index(lower, "balances"); idx != -1 { rest := strings.TrimSpace(key[idx+len("balances"):]) rest = strings.TrimLeft(rest, "/") if rest != "" { if i := strings.Index(rest, "/"); i != -1 { return rest[:i] } return rest } } parts := strings.Split(key, "/") if len(parts) > 1 { return parts[1] } return "" Missing Height Use fetchBalanceData receives height but never uses it; if historical queries are expected, consider documenting or enforcing behavior. Otherwise, remove the parameter to avoid confusion. func (c *remoteClient) fetchBalanceData(ctx context.Context, key string, height int64) ([]byte, error) { address := c.extractAddressFromKey(key) if address == "" { return nil, nil } req := &types.QueryBalancesRequest{ Address: address, } resp, err := c.queryClient.Balances(ctx, req) if err != nil { if isNotFoundErr(err) { return nil, nil } return nil, fmt.Errorf("gRPC balances query failed: %w", err) } return c.codec.Marshal(resp) }

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Refactor repeated logic into a loop Refactor the extractAddressFromKey function to eliminate code duplication by using a helper function and iterating over a list of key prefixes. x/thorchain/forking/client.go [615-664] - if idx := strings.Index(lower, "account/"); idx != -1 { - rest := strings.Trim(key[idx+len("account/"):], "/") - if rest != "" { - if i := strings.Index(rest, "/"); i != -1 { - return rest[:i] - } - return rest + prefixesWithSlash := []string{"account/", "balances/", "balance/"} + for _, prefix := range prefixesWithSlash { + if addr := c.extractAddressAfterPrefix(key, lower, prefix, true); addr != "" { + return addr } } - if idx := strings.Index(lower, "balances/"); idx != -1 { - rest := strings.Trim(key[idx+len("balances/"):], "/") - if rest != "" { - if i := strings.Index(rest, "/"); i != -1 { - return rest[:i] - } - return rest + prefixesWithoutSlash := []string{"account", "balances"} + for _, prefix := range prefixesWithoutSlash { + if addr := c.extractAddressAfterPrefix(key, lower, prefix, false); addr != "" { + return addr } } - if idx := strings.Index(lower, "balance/"); idx != -1 { - rest := strings.Trim(key[idx+len("balance/"):], "/") - if rest != "" { - if i := strings.Index(rest, "/"); i != -1 { - return rest[:i] - } - return rest - } +// Helper function to be added to the remoteClient struct +func (c *remoteClient) extractAddressAfterPrefix(key, lower, prefix string, trimSlashes bool) string { + idx := strings.Index(lower, prefix) + if idx == -1 { + return "" } - if idx := strings.Index(lower, "account"); idx != -1 { - rest := strings.TrimSpace(key[idx+len("account"):]) + rest := key[idx+len(prefix):] + if trimSlashes { + rest = strings.Trim(rest, "/") + } else { + rest = strings.TrimSpace(rest) rest = strings.TrimLeft(rest, "/") - if rest != "" { - if i := strings.Index(rest, "/"); i != -1 { - return rest[:i] - } - return rest - } - } - if idx := strings.Index(lower, "balances"); idx != -1 { - rest := strings.TrimSpace(key[idx+len("balances"):]) - rest = strings.TrimLeft(rest, "/") - if rest != "" { - if i := strings.Index(rest, "/"); i != -1 { - return rest[:i] - } - return rest - } } + if rest != "" { + if i := strings.Index(rest, "/"); i != -1 { + return rest[:i] + } + return rest + } + return "" +} + [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies duplicated logic and proposes a valid refactoring to improve code maintainability and readability by using a helper function and iterating over prefixes.	Low
More