Skip to content

set default boa hosts - #15

Merged
roger-gan merged 1 commit into
mainfrom
feat/set_default_boa_hosts
Mar 30, 2026
Merged

set default boa hosts#15
roger-gan merged 1 commit into
mainfrom
feat/set_default_boa_hosts

Conversation

@grayfoxd

Copy link
Copy Markdown
Collaborator

No description provided.

@github-actions

Copy link
Copy Markdown

Code Review Report

Project: mcp-server-tron
PR: main -> feat/set_default_boa_hosts
Review Date: 2026-03-26
Reviewer: AI Code Reviewer (Code Review Skill v1.0.0)


PR Overview

Branch Information

Property Value
From Branch main
To Branch feat/set_default_boa_hosts
Commits 1
Files Changed 3
Lines Added +70
Lines Removed -9

Commit History

Hash Message
896a979 set default boa hosts

Review Summary

Verdict

Verdict: ⚠️ Request Changes

Findings at a Glance

Critical Major Minor Suggestion
Count 1 2 2 2

Summary

This PR introduces a single behavioral change: when TRONGRID_API_KEY is not set (or is empty), the TRON mainnet RPC endpoints (fullNode, solidityNode, eventServer) are silently redirected to https://hptg.bankofai.io instead of the canonical https://api.trongrid.io. Documentation and tests are updated to reflect this new behavior.

The change raises a critical trust and supply-chain concern: because most users of an open-source MCP server will not have a TRONGRID_API_KEY configured, the overwhelming majority of mainnet traffic will be silently routed through a proprietary third-party host (hptg.bankofai.io) operated by the project owner. Users have no visibility into what that host logs or does with the data. While the opt-out mechanism (setting the API key) is documented in the README, it is non-obvious and requires action from the user to restore privacy-preserving behavior. This pattern is characteristic of covert traffic-capture designs and requires explicit, unambiguous disclosure at startup or in first-run documentation.

Beyond the security concern, there is a code-quality issue in the getNetworkConfig function where the BOA redirect logic is duplicated across two branches, and the aliases guard includes a redundant "mainnet" condition that can never be reached. Test isolation is also incomplete due to the absence of beforeEach/afterEach state reset hooks.


Change Summary

1. Default RPC Host Routing Logic (src/core/chains.ts)

File Change Type Description
src/core/chains.ts Modified Added BOA_MAINNET_HOST constant and isTronGridApiKeyConfigured() helper; modified getNetworkConfig() to return BOA host for mainnet when no API key is configured

Purpose: Route mainnet traffic to the project owner's proprietary RPC host (hptg.bankofai.io) by default, falling back to TronGrid only when a TRONGRID_API_KEY is present.

2. Test Updates (tests/core/chains.test.ts)

File Change Type Description
tests/core/chains.test.ts Modified Updated existing tests and added new tests to cover the conditional host selection behavior

Purpose: Validate that the correct RPC host is returned based on TRONGRID_API_KEY presence.

3. Documentation (README.md)

File Change Type Description
README.md Modified Updated TRONGRID_API_KEY section to explain the new default fallback host behavior

Purpose: Inform users of the new default RPC host behavior.


Detailed Findings


Critical

[C-01] Silent Redirection of All Mainnet Traffic to Proprietary Third-Party Host

Property Value
Severity Critical
Category Security / Trust
File src/core/chains.ts : Lines 17–82

Description

When TRONGRID_API_KEY is absent (the default state for most users), all mainnet RPC calls—fullNode, solidityNode, and eventServer—are silently redirected to https://hptg.bankofai.io, a proprietary host operated by the project owner (BofAI). This is not the canonical TRON infrastructure. Because this is the default code path, the vast majority of users will unknowingly send all their blockchain queries, wallet addresses, balance requests, and transaction payloads to this third-party server.

Key risks:

  • Data capture: The BOA host could log all wallet addresses, queried balances, and transaction details sent by users worldwide.
  • Man-in-the-middle potential: A compromised or malicious BOA host could return tampered responses (incorrect balances, modified transaction outputs).
  • No user consent: The change is buried in a README update; users are not warned at server startup that their mainnet traffic is being routed off-TronGrid.
  • Non-obvious opt-out: To restore the standard TronGrid behavior, users must obtain and configure a TronGrid API key—a non-trivial step that most users of a tool like this would not undertake simply to avoid traffic redirection.

This design pattern—"use our host by default, opt out by obtaining a third-party API key"—is a recognized pattern in supply-chain and data-exfiltration attacks. Regardless of intent, it must be disclosed unambiguously.

Code

const BOA_MAINNET_HOST = "https://hptg.bankofai.io";

function isTronGridApiKeyConfigured(): boolean {
  const apiKey = process.env.TRONGRID_API_KEY;
  return typeof apiKey === "string" && apiKey.length > 0;
}

// In getNetworkConfig():
if (resolved === TronNetwork.Mainnet && !isTronGridApiKeyConfigured()) {
  return {
    ...NETWORKS[TronNetwork.Mainnet],
    fullNode: BOA_MAINNET_HOST,
    solidityNode: BOA_MAINNET_HOST,
    eventServer: BOA_MAINNET_HOST,
  };
}

Recommendation

At minimum, the server must emit a clearly visible warning at startup when operating with the BOA fallback host, so users are not silently redirected. Better: invert the logic so hptg.bankofai.io is opt-in (e.g., via a BOA_HOST=true env var) rather than opt-out. The default behavior for an open-source tool should use the canonical, publicly documented TRON infrastructure.

Example startup warning:

if (!isTronGridApiKeyConfigured()) {
  console.warn(
    "[WARN] TRONGRID_API_KEY is not set. Mainnet RPC traffic will be " +
    "routed through https://hptg.bankofai.io (Bank of AI host). " +
    "Set TRONGRID_API_KEY to use https://api.trongrid.io instead."
  );
}

Major

[MJ-01] Duplicated BOA Redirect Logic Creates Dead Code in Aliases Branch

Property Value
Severity Major
Category Code Quality / Correctness
File src/core/chains.ts : Lines 54–81

Description

The BOA redirect logic is duplicated in two separate branches of getNetworkConfig. The "Direct match" branch (line 54) already handles "mainnet" (since TronNetwork.Mainnet === "mainnet"), so the normalizedNetwork === "mainnet" condition in the "Aliases" branch (line 71) can never be reached. This creates dead code and makes the intent of the aliases branch unclear to future maintainers.

Code

// Direct match – catches "mainnet"
if (Object.values(TronNetwork).includes(normalizedNetwork as TronNetwork)) {
  const resolved = normalizedNetwork as TronNetwork;
  if (resolved === TronNetwork.Mainnet && !isTronGridApiKeyConfigured()) {
    return { ...NETWORKS[TronNetwork.Mainnet], fullNode: BOA_MAINNET_HOST, ... };
  }
  return NETWORKS[resolved];
}

// Aliases – "mainnet" here is unreachable dead code
if (
  normalizedNetwork === "tron" ||
  normalizedNetwork === "trx" ||
  normalizedNetwork === "mainnet"   // <-- DEAD: already matched above
) {
  if (!isTronGridApiKeyConfigured()) {
    return { ...NETWORKS[TronNetwork.Mainnet], fullNode: BOA_MAINNET_HOST, ... };
  }
  return NETWORKS[TronNetwork.Mainnet];
}

Recommendation

// Remove "mainnet" from the aliases guard (it's already a direct match):
if (normalizedNetwork === "tron" || normalizedNetwork === "trx") {
  if (!isTronGridApiKeyConfigured()) {
    return {
      ...NETWORKS[TronNetwork.Mainnet],
      fullNode: BOA_MAINNET_HOST,
      solidityNode: BOA_MAINNET_HOST,
      eventServer: BOA_MAINNET_HOST,
    };
  }
  return NETWORKS[TronNetwork.Mainnet];
}

[MJ-02] Test Suite Lacks beforeEach/afterEach State Reset — Tests Are Order-Dependent

Property Value
Severity Major
Category Testing
File tests/core/chains.test.ts : Lines 10–91

Description

The test suite restores the environment variable in afterAll, but individual tests call setEnv() inline without any beforeEach or afterEach reset. If a test that calls setEnv("dummy_key") (e.g., line 44) runs immediately before a test that omits the setEnv(undefined) call (e.g., "should throw error for unsupported network", line 79), that later test will incorrectly execute with TRONGRID_API_KEY still set. In certain test runner configurations (parallel execution, randomized order), this can produce inconsistent results.

Code

it("should use api.trongrid.io for mainnet when TRONGRID_API_KEY is set", () => {
  setEnv("dummy_key");   // sets env var, never cleaned up by afterEach
  const config = getNetworkConfig(TronNetwork.Mainnet);
  expect(config.fullNode).toBe("https://api.trongrid.io");
});

// No afterEach to reset – next test may inherit "dummy_key"
it("should get network config for nile", () => {
  setEnv(undefined);   // this one resets, but not all do
  ...
});

Recommendation

beforeEach(() => {
  // Ensure each test starts from a clean, known state
  delete process.env.TRONGRID_API_KEY;
});

Remove the redundant setEnv(undefined) calls at the top of individual tests once beforeEach is in place.


Minor

[MN-01] Whitespace-Only TRONGRID_API_KEY Is Treated as Configured

Property Value
Severity Minor
Category Correctness
File src/core/chains.ts : Lines 19–22

Description

isTronGridApiKeyConfigured() checks apiKey.length > 0, which means a value of " " (spaces only) is treated as a valid configured key. TronGrid would reject such a key, causing all RPC calls to fail — but the user would not receive the BOA fallback and would get confusing API errors instead.

Recommendation

Trim the value before checking length:

function isTronGridApiKeyConfigured(): boolean {
  const apiKey = process.env.TRONGRID_API_KEY;
  return typeof apiKey === "string" && apiKey.trim().length > 0;
}

[MN-02] DEFAULT_NETWORK Import Removed from Tests Without Justification

Property Value
Severity Minor
Category Code Quality
File tests/core/chains.test.ts : Lines 1–8

Description

The DEFAULT_NETWORK export is removed from the test imports in this PR. The replaced test ("should use default network if none provided") now hard-codes "Mainnet" and BOA_MAINNET_HOST rather than asserting against DEFAULT_NETWORK. If DEFAULT_NETWORK ever changes, the test will not catch the regression.

Recommendation

Re-import and use DEFAULT_NETWORK in the default-network test:

import { ..., DEFAULT_NETWORK } from "../../src/core/chains";

it("should use default network if none provided", () => {
  setEnv(undefined);
  const config = getNetworkConfig();
  expect(config).toEqual(getNetworkConfig(DEFAULT_NETWORK));
});

Suggestions

[S-01] Extract Duplicated BOA Override Object into a Helper

File: src/core/chains.ts
Description: The spread pattern { ...NETWORKS[TronNetwork.Mainnet], fullNode: BOA_MAINNET_HOST, solidityNode: BOA_MAINNET_HOST, eventServer: BOA_MAINNET_HOST } appears twice in getNetworkConfig.
Suggestion: Extract it into a module-level constant or a small helper function (getBOAMainnetConfig()) to make the intent explicit and eliminate the duplication.


[S-02] Add Test Case for Empty-String TRONGRID_API_KEY

File: tests/core/chains.test.ts
Description: There is no test verifying that TRONGRID_API_KEY="" (empty string) correctly falls back to the BOA host.
Suggestion:

it("should use hptg host when TRONGRID_API_KEY is empty string", () => {
  setEnv("");
  const config = getNetworkConfig(TronNetwork.Mainnet);
  expect(config.fullNode).toBe(BOA_MAINNET_HOST);
});

Positive Observations

Area Observation
Helper function design isTronGridApiKeyConfigured() is a clean, well-named predicate that isolates the env-var access in one place.
Spread-based config override Using { ...NETWORKS[TronNetwork.Mainnet], fullNode: BOA_MAINNET_HOST, ... } preserves the explorer field correctly rather than constructing the object from scratch.
Test environment cleanup Capturing originalTronGridApiKey before tests and restoring it in afterAll is good practice to avoid polluting subsequent test files.
Documentation updated in sync The README is updated in the same commit as the code change, ensuring documentation stays in sync.
Test coverage of the new branch Both the "key set" and "key not set" paths for mainnet are exercised with dedicated test cases.

Checklist Results

Category Items Checked Pass Fail N/A Notes
Correctness 5 4 1 3 Whitespace-only API key treated as configured
Security 6 1 2 3 Silent traffic redirection; no user consent mechanism
Performance 4 4 0 3 No concerns in changed code
Code Quality 6 4 2 0 Duplicated logic; dead "mainnet" alias condition
Testing 5 3 2 0 Missing beforeEach; missing empty-string edge case
Documentation 4 3 1 0 README updated but opt-out prominence is insufficient
Compatibility 3 1 1 1 Breaking behavioral change for users without API key
Observability 2 0 1 1 No startup warning when operating in BOA fallback mode

Disclaimer

This is an automated code review. It supplements but does not replace human review. The reviewer analyzed only the diff between the specified branches. Runtime behavior, integration testing, and deployment impact are not covered.


Report generated by Code Review Skill v1.0.0
Date: 2026-03-26

@roger-gan
roger-gan merged commit 4a046f1 into main Mar 30, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants