Forking: bank keeper with local-first reads and TLS gRPC fallback; wire in app; scaffold forking KV under x/bloctopus

[tiljrd]

User description

Forking: Bank keeper with local-first reads and TLS gRPC fallback

Summary

Implements a ForkingBankKeeper under x/bloctopus/forkbank that provides local-first balance and metadata queries with remote mainnet fallback via TLS gRPC to grpc.thor.pfc.zone:443. When querying balances or denom metadata, the keeper first checks local storage and falls back to querying mainnet if the data doesn't exist locally. For write operations like SendCoins, the keeper relies on the base keeper's internal logic which will call back into our overridden GetBalance methods to materialize remote balances locally when needed.

Key changes:

• ForkingBankKeeper: Embeds bankkeeper.BaseKeeper and implements bankkeeper.Keeper interface
• Remote client: Uses TLS gRPC to query mainnet bank balances and denom metadata
• App wiring: Changed THORChainApp.BankKeeper from concrete BaseKeeper to Keeper interface
• Forking scaffolds: Added empty scaffold files under x/bloctopus/forking for future KV store functionality

Review & Testing Checklist for Human

🔴 HIGH RISK - Core banking functionality changes

• End-to-end forking test: Deploy with Kurtosis, query balances for addresses that don't exist locally, verify remote fallback works
• Write path materialization: Send coins between addresses, verify sender/recipient balances are properly materialized from mainnet then updated locally
• Network failure resilience: Test behavior when grpc.thor.pfc.zone:443 is unreachable - should gracefully return empty results
• HasBalance logic verification: Check that the HasBalance(addr, NewCoin(denom, NewInt(1))) logic correctly distinguishes local vs remote accounts across different scenarios
• Interface compatibility: Verify all bank keeper methods are properly forwarded and no existing functionality is broken (especially staking, mint, authz modules)

Notes

• Remote endpoint is currently hardcoded to grpc.thor.pfc.zone:443
• The x/bloctopus/forking files are mostly empty scaffolds and not used yet
• No unit tests were added - recommend adding tests for the forking logic before merge
• This implements the core requirement from user Til Jordan (@tiljrd) for "forking" capabilities that pull mainnet data on-demand

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

---

PR Type

Enhancement

---

Description

• Implement ForkingBankKeeper with local-first balance queries

• Add TLS gRPC fallback to mainnet for missing data

• Wire bank keeper as interface in app configuration

• Scaffold forking KV store infrastructure under x/bloctopus

---

Diagram Walkthrough

flowchart LR
  App["THORChain App"] --> FBK["ForkingBankKeeper"]
  FBK --> Local["Local Storage"]
  FBK --> Remote["Remote gRPC Client"]
  Remote --> Mainnet["grpc.thor.pfc.zone:443"]
  Local -- "if data exists" --> Return["Return Local Data"]
  Remote -- "if local missing" --> Return

Loading

File Walkthrough

	Relevant files
Configuration changes	2 files app.go Wire ForkingBankKeeper as interface in app                              +11/-2    flags.go Command line flags for forking configuration                          +13/-0
Enhancement	8 files keeper.go Core forking bank keeper implementation                                    +82/-0    remote.go TLS gRPC client for mainnet queries                                            +72/-0    types.go Type definitions for forking keeper                                            +15/-0    gas.go Gas meter wrapper for SDK integration                                        +22/-0    iterator.go Merged iterator scaffold for KV operations                              +14/-0    service.go KV store service wrapper scaffold                                                +15/-0    store.go KV store wrapper scaffold                                                                +11/-0    types.go Configuration types for forking options                                    +10/-0
Miscellaneous	2 files iterator.go Empty iterator scaffold file                                                          +1/-0      cache.go Empty cache scaffold file                                                                +1/-0

---

[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: 3 🔵🔵🔵⚪⚪

🧪 No relevant tests

🔒 Security concerns TLS configuration: The gRPC client uses a default tls.Config without hostname or CA pinning customization. While this enables TLS, it may not enforce stricter verification (e.g., custom roots, min TLS versions). Also, lack of dial timeouts could be exploited to cause resource hangs. Consider specifying reasonable timeouts and optionally validating certificates against expected SANs/roots.

⚡ Recommended focus areas for review Possible Issue The local-vs-remote check in balance queries relies on HasBalance with an amount of 1 for a given denom, which may yield false negatives/positives for accounts with zero balance locally but non-zero remotely, or for different default denoms. Verify this logic aligns with intended "local-first" behavior across all denoms and zero-balance cases. func (k ForkingBankKeeper) GetAllBalances(ctx context.Context, addr sdk.AccAddress) sdk.Coins { if k.BaseKeeper.HasBalance(ctx, addr, sdk.NewCoin("rune", math.NewInt(1))) { return k.BaseKeeper.GetAllBalances(ctx, addr) } resp, err := k.client.RemoteBalances(ctx, addr.String()) if err != nil || resp == nil { return sdk.NewCoins() } return sdk.NewCoins(resp.Balances...) } func (k ForkingBankKeeper) GetBalance(ctx context.Context, addr sdk.AccAddress, denom string) sdk.Coin { if k.BaseKeeper.HasBalance(ctx, addr, sdk.NewCoin(denom, math.NewInt(1))) { return k.BaseKeeper.GetBalance(ctx, addr, denom) } resp, err := k.client.RemoteBalances(ctx, addr.String()) if err != nil { return sdk.NewCoin(denom, math.ZeroInt()) } for _, c := range resp.Balances { if c.Denom == denom { return c } } return sdk.NewCoin(denom, math.ZeroInt()) } Resilience gRPC client lacks timeouts, context deadlines, and retry/backoff. Network hiccups could block or degrade queries. Consider grpc.DialOptions (e.g., WithBlock, backoff, per-RPC creds), and enforcing per-call timeouts via context to avoid hanging critical paths. func (c *RemoteClient) ensureConn() error { if c.dialed { return nil } creds := credentials.NewTLS(&tls.Config{}) conn, err := grpc.Dial(c.endpoint, grpc.WithTransportCredentials(creds)) if err != nil { return err } c.conn = conn c.bankQuery = banktypes.NewQueryClient(conn) c.dialed = true return nil } func (c *RemoteClient) Close() { if c.conn != nil { _ = c.conn.Close() } } func (c *RemoteClient) RemoteBalances(ctx context.Context, addr string) (*banktypes.QueryAllBalancesResponse, error) { if err := c.ensureConn(); err != nil { return nil, err } resp, err := c.bankQuery.AllBalances(ctx, &banktypes.QueryAllBalancesRequest{Address: addr}) if err != nil { return nil, err } return resp, nil } func (c *RemoteClient) RemoteDenomsMetadata(ctx context.Context) (*banktypes.QueryDenomsMetadataResponse, error) { if err := c.ensureConn(); err != nil { return nil, err } resp, err := c.bankQuery.DenomsMetadata(ctx, &banktypes.QueryDenomsMetadataRequest{}) if err != nil { return nil, err } return resp, nil Config Hardcode The remote endpoint is hardcoded at initialization. Consider wiring from flags/env (forking flags exist) and validating TLS settings to avoid inflexibility and misconfiguration in different environments. fbk, err := forkbankkeeper.NewForkingBankKeeper(baseBank, forkbankkeeper.Config{ Endpoint: "grpc.thor.pfc.zone:443", }) if err != nil { panic(err) } app.BankKeeper = fbk

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	The forking logic is fundamentally flawed The current forking logic incorrectly chooses between local or remote balances instead of merging them. It should be redesigned to combine both sources, using the local state as an overlay on the remote state to ensure a complete and accurate balance view. Examples: x/bloctopus/forkbank/keeper/keeper.go [24-33] func (k ForkingBankKeeper) GetAllBalances(ctx context.Context, addr sdk.AccAddress) sdk.Coins { if k.BaseKeeper.HasBalance(ctx, addr, sdk.NewCoin("rune", math.NewInt(1))) { return k.BaseKeeper.GetAllBalances(ctx, addr) } resp, err := k.client.RemoteBalances(ctx, addr.String()) if err != nil || resp == nil { return sdk.NewCoins() } return sdk.NewCoins(resp.Balances...) } x/bloctopus/forkbank/keeper/keeper.go [35-49] func (k ForkingBankKeeper) GetBalance(ctx context.Context, addr sdk.AccAddress, denom string) sdk.Coin { if k.BaseKeeper.HasBalance(ctx, addr, sdk.NewCoin(denom, math.NewInt(1))) { return k.BaseKeeper.GetBalance(ctx, addr, denom) } resp, err := k.client.RemoteBalances(ctx, addr.String()) if err != nil { return sdk.NewCoin(denom, math.ZeroInt()) } for _, c := range resp.Balances { if c.Denom == denom { ... (clipped 5 lines) Solution Walkthrough: Before: // In x/bloctopus/forkbank/keeper/keeper.go func (k ForkingBankKeeper) GetAllBalances(ctx context.Context, addr sdk.AccAddress) sdk.Coins { // Check for a local balance of a specific coin ('rune'). if k.BaseKeeper.HasBalance(ctx, addr, sdk.NewCoin("rune", math.NewInt(1))) { // If it exists, return ONLY local balances. return k.BaseKeeper.GetAllBalances(ctx, addr) } // Otherwise, fetch and return ONLY remote balances. resp, err := k.client.RemoteBalances(ctx, addr.String()) if err != nil { return sdk.NewCoins() } return sdk.NewCoins(resp.Balances...) } After: // In x/bloctopus/forkbank/keeper/keeper.go func (k ForkingBankKeeper) GetAllBalances(ctx context.Context, addr sdk.AccAddress) sdk.Coins { // 1. Fetch remote balances as the base state. remoteCoins := sdk.NewCoins() resp, err := k.client.RemoteBalances(ctx, addr.String()) if err == nil && resp != nil { remoteCoins = sdk.NewCoins(resp.Balances...) } // 2. Fetch local balances which act as an overlay/delta. localCoins := k.BaseKeeper.GetAllBalances(ctx, addr) // 3. Merge the two, with local balances overriding remote ones. // This creates a unified, complete view of the account's state. mergedCoins := remoteCoins.Add(localCoins...) // Or a more sophisticated merge return mergedCoins } Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a critical design flaw in the core forking logic, where choosing one data source over another instead of merging them leads to an incomplete and incorrect view of account balances.	High
General	Avoid hardcoding the gRPC endpoint Avoid hardcoding the gRPC endpoint in app.go. Instead, retrieve the endpoint from appOpts to allow for runtime configuration, aligning with the new flags introduced in the PR. app/app.go [318-323] + forkingGRPC, _ := appOpts.Get("forking.grpc").(string) fbk, err := forkbankkeeper.NewForkingBankKeeper(baseBank, forkbankkeeper.Config{ - Endpoint: "grpc.thor.pfc.zone:443", + Endpoint: forkingGRPC, }) if err != nil { panic(err) } Apply / Chat Suggestion importance[1-10]: 8 __ Why: This is a high-impact suggestion that correctly identifies a hardcoded value that should be configurable, especially since the PR adds flags for this purpose. Applying this change significantly improves the feature's flexibility and usability.	Medium
	Optimize remote balance fetching logic Optimize the GetBalance function by fetching only the specific balance required from the remote endpoint, instead of fetching all balances and filtering locally. This requires adding a new method to RemoteClient to query for a single denomination. x/bloctopus/forkbank/keeper/keeper.go [35-49] func (k ForkingBankKeeper) GetBalance(ctx context.Context, addr sdk.AccAddress, denom string) sdk.Coin { if k.BaseKeeper.HasBalance(ctx, addr, sdk.NewCoin(denom, math.NewInt(1))) { return k.BaseKeeper.GetBalance(ctx, addr, denom) } - resp, err := k.client.RemoteBalances(ctx, addr.String()) - if err != nil { + // Assumes a new `RemoteBalance` method is added to the client for single denom queries. + resp, err := k.client.RemoteBalance(ctx, addr.String(), denom) + if err != nil || resp == nil || resp.Balance == nil { return sdk.NewCoin(denom, math.ZeroInt()) } - for _, c := range resp.Balances { - if c.Denom == denom { - return c - } - } - return sdk.NewCoin(denom, math.ZeroInt()) + + return *resp.Balance } Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out a performance inefficiency in GetBalance and proposes a valid optimization by using a more specific gRPC call, which would reduce network overhead.	Medium
	Use the insecure flag for TLS Use the tlsInsecure field in RemoteClient to conditionally set InsecureSkipVerify in the tls.Config for the gRPC connection, allowing for more flexible TLS configurations. x/bloctopus/forkbank/keeper/remote.go [31-44] func (c *RemoteClient) ensureConn() error { if c.dialed { return nil } - creds := credentials.NewTLS(&tls.Config{}) + creds := credentials.NewTLS(&tls.Config{ + InsecureSkipVerify: c.tlsInsecure, + }) conn, err := grpc.Dial(c.endpoint, grpc.WithTransportCredentials(creds)) if err != nil { return err } c.conn = conn c.bankQuery = banktypes.NewQueryClient(conn) c.dialed = true return nil } Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that the tlsInsecure field is unused and proposes to use it for InsecureSkipVerify, which improves the feature's flexibility for testing and development environments.	Low
More