Skip to content

feat(API): refactor merge rpc - #17

Open
SeriousCoding789 wants to merge 16 commits into
SeriousCoding789:developfrom
Little-Peony:refactor_merge_rpc
Open

feat(API): refactor merge rpc#17
SeriousCoding789 wants to merge 16 commits into
SeriousCoding789:developfrom
Little-Peony:refactor_merge_rpc

Conversation

@SeriousCoding789

@SeriousCoding789 SeriousCoding789 commented Aug 20, 2026

Copy link
Copy Markdown
Owner

What does this PR do?

Implements tronprotocol#6927 — the gRPC counterpart of the HTTP servlet dedup in tronprotocol#6922.

RpcApiServiceOnSolidity and RpcApiServiceOnPBFT each re-declare the whole read surface as per-method delegations whose only job is to switch the per-thread read cursor:

@Override
public void getAccount(Account req, StreamObserver<Account> obs) {
  walletOnSolidity.futureGet(() -> rpcApiService.getWalletSolidityApi().getAccount(req, obs));
}

There are exactly 100 of these — 51 in the Solidity service, 49 in the PBFT one — spread over four inner service classes. This PR replaces them with a cursor-parameterized ServerInterceptor:

  • Adds CursorServerInterceptor with SolidityCursorInterceptor / PbftCursorInterceptor. It brackets Listener.onHalfClose() — the callback gRPC runs a unary handler inline from — setting the cursor before and restoring it in a finally.
  • The two cursor services now register the base service's DatabaseApi / WalletSolidityApi singletons directly, wrapped in ServerInterceptors.intercept(...). RpcApiServiceOnSolidity goes from 488 lines to 36, RpcApiServiceOnPBFT from 495 to 36.
  • A second, independent axis of duplication inside RpcApiService itself: WalletSolidityApi (serving protocol.WalletSolidity) re-implements read handlers WalletApi (serving protocol.Wallet) already has. 41 of its 47 methods now delegate to the shared WalletApi singleton; the remaining 6 already routed through shared *Common / callContract helpers, so no duplicated handler body is left. Java is single-inheritance and gRPC generates one ImplBase per proto service, so both classes have to stay — only the bodies go.
  • Fixes the double-close on the error path, which the dedup made visible. A handler that calls responseObserver.onError(...) and then falls through to responseObserver.onCompleted() closes the call twice; the second close() hits checkState(!closeCalled, "call already closed") in gRPC's ServerCallImpl and throws. RpcApiService had 32 handlers shaped that way on develop — 8 disappear with the duplicated WalletSolidityApi bodies, and the remaining 24 get an explicit return, the shape the file already used elsewhere. The file now has none.

10 files changed, +945 / −1267.

Why are these changes required?

A read handler currently lives in up to four places, so one RPC change has to be mirrored four times, and missing one makes the same RPC behave differently depending on which port a client hits.

That has already happened. Of the 41 handlers being dedup'd inside RpcApiService, 23 are byte-identical between the two copies on develop and 18 differ. Of those 18:

  • 8 are hardening that only ever landed in WalletApi — it gained a return after responseObserver.onError(...) and WalletSolidityApi did not, so on the error path the copy serving the Solidity and PBFT ports falls through to onCompleted() and terminates the call twice (getMerkleTreeVoucherInfo, isSpend, scanAndMarkNoteByIvk, scanNoteByIvk, scanNoteByOvk, isShieldedTRC20ContractNoteSpent, scanShieldedTRC20NotesByIvk, scanShieldedTRC20NotesByOvk). The last two additionally have a BadItemException | ZksnarkException branch with logging that only WalletApi ever received.
  • 7 are cosmetic — a parameter name, a temporary variable, line wrapping.
  • 1 differs only in a log prefix.
  • 2 (getBlockByNum, getBlockByNum2) drifted the other way: WalletSolidityApi has a num >= 0 guard WalletApi lacks.

None of that was written deliberately; it is what happens when the same handler exists four times.

Lining the two copies up also showed that the return hardening was never finished on WalletApi either — 24 more handlers in the same file still fall through — so this PR completes it rather than leaving the file in two states.

Behaviour differences vs develop

Method sets, per port. The base WalletSolidityApi (47 methods) and DatabaseApi (4) expose exactly the same methods before and after — only bodies changed.

Port Before After Delta
HEAD (RpcApiService) 47 + 4 47 + 4 none
SOLIDITY 47 + 4 47 + 4 none
PBFT 45 + 4 47 + 4 +2

The PBFT port gains getPaginatedNowWitnessList and getTransactionInfoByBlockNum — the only two methods RpcApiServiceOnPBFT never mirrored from RpcApiServiceOnSolidity; they returned UNIMPLEMENTED there before. Both are ordinary reads and resolve against the PBFT snapshot like every other read on that port.

Handler bodies. Only three WalletApi bodies changed beyond the added returns:

  • getBlockByNum / getBlockByNum2 adopt the solidity copy's num >= 0 guard. No response changeWallet#getBlockByNum already catches the StoreException and returns null for a negative number, so both paths reach onNext(null); the guard only skips a futile store lookup and its log line.
  • getAssetIssueByName drops the "FullNode " prefix from one logger.debug line, which was the only difference between the two copies.

Error paths. Every handler that used to emit onError followed by onCompleted now emits a single terminal event, on all three ports. Not visible to clientsonError had already closed the call with the error status and the second close() threw before sending anything; what goes away is one server-side IllegalStateException per failed call. Two groups:

  • Fixed as a side effect of the dedup, on the SOLIDITY and PBFT ports: the 8 shielded handlers listed above. Worth noting these were not a rare corner — the first statement of the five sapling reads in Wallet is checkAllowShieldedTransactionApi(), and node.allowShieldedTransactionApi defaults to false, so on a default node every such call took the double-close path.
  • Fixed explicitly, on all three ports: 17 handlers in WalletApi (getPaginatedNowWitnessList, getTransactionInfoByBlockNum, getDelegatedResourceV2, getDelegatedResourceAccountIndex, getDelegatedResourceAccountIndexV2, getCanDelegatedMaxSize, getCanWithdrawUnfreezeAmount, getAvailableUnfreezeCount, getBandwidthPrices, getEnergyPrices, getMemoFee, getNodeInfo, getMarketOrderByAccount, getMarketOrderById, getMarketOrderListByPair, getMarketPriceByPair, getMarketPairList) and 7 shared helpers reachable from both services (getBlockCommon, getRewardInfoCommon, getBrokerageInfoCommon, getBurnTrxCommon, getPendingSizeCommon, getTransactionFromPendingCommon, getTransactionListFromPendingCommon).

Cursor semantics are unchanged. Manager#setCursor still computes the headNum - pbftNum offset for PBFT, exactly as WalletOnPBFT.futureGet did.

Scope. gRPC only. WalletOnCursor / WalletOnSolidity / WalletOnPBFT stay, because the HTTP and JSON-RPC servlets still call futureGet; removing those is a separate change. Ports, switches, proto definitions and the server-level interceptor chain (rate limiter, api access, lite-fullnode filter, prometheus) are untouched.

This PR has been tested by:

  • Unit Tests — 7 new tests plus two assertions added to the existing end-to-end suite, all passing. Each pins something that can actually go wrong:
    • CursorInterceptorScopeTest (2) drives interceptCall() on one thread and the returned listener's onHalfClose() on another — which is what gRPC's SerializingExecutor is free to do. An implementation that scoped the cursor around interceptCall fails here by construction rather than by luck. Also covers the finally reset when the handler throws.
    • CursorInterceptorServerTest (1) runs the production interceptor behind a real gRPC server and asserts the cursor is set and restored exactly once, on the handler's own thread. This is the end-to-end half: the whole design rests on gRPC running the handler inline from onHalfClose, and if that stops holding the cursor never reaches the read path and the port serves HEAD data with no error.
    • CursorInterceptorWiringTest (2) runs the real addService of both cursor services against a mock builder and asserts each shared read service is registered as an intercepted definition. Dropping ServerInterceptors.intercept leaves every other test green while the port silently serves HEAD, so this is the gRPC counterpart of CursorFilterInstallationTest.
    • RpcApiServiceErrorPathTest (2) drives every unary handler of WalletApi and WalletSolidityApi with collaborators that throw, and asserts none of them terminates the call more than once. Reverting the added returns makes it fail on getDelegatedResourceV2, getPendingSize and getBlock, so it reaches the shared *Common helpers as well.
    • RpcApiServicesTest — the existing suite drives all three ports end to end over 129 tests; it now also calls getPaginatedNowWitnessList and getTransactionInfoByBlockNum on the PBFT stub, pinning the one intentional behaviour change.
  • Manual Testing

@coderabbitai

coderabbitai Bot commented Aug 20, 2026

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Team

Run ID: 863877de-ebfa-417b-aa07-37d61561c7fa


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@SeriousCoding789 SeriousCoding789 changed the title fix: refactor merge rpc feat(API): refactor merge rpc Aug 21, 2026
@SeriousCoding789
SeriousCoding789 changed the base branch from feat/refactor_merge_http_servlets to develop August 24, 2026 06:02
@Little-Peony
Little-Peony force-pushed the refactor_merge_rpc branch 2 times, most recently from 6d6034d to 8b15128 Compare September 1, 2026 03:09
Delegate 17 more WalletSolidityApi read methods to the WalletApi
singleton so each handler body lives once. 41/47 methods now delegate;
the other 6 already share outer *Common helpers.

Behavior notes:
- getBlockByNum/getBlockByNum2: WalletApi adopts the guarded version,
  so the FullNode HEAD port now returns null for negative num.
- getMerkleTreeVoucherInfo, isSpend, scanNoteByIvk/ByOvk,
  scanAndMarkNoteByIvk and the 3 shielded-TRC20 methods: solidity error
  paths now match WalletApi (return after onError; the two
  scanShieldedTRC20Notes* also add error logging). Client-observable
  behavior unchanged.
- getAssetIssueByName: solidity debug log label Solidity -> FullNode.

Still TODO: build verification and tests for the HEAD getBlockByNum
guard change.
Attach the cursor interceptor with ServerInterceptors.intercept on the two
shared service definitions instead of serverBuilder.intercept on the port.

The call order a read sees is unchanged either way — the cursor still runs
innermost, immediately before the handler — but the reason it does becomes
structural rather than an assumption about registration order, and the
interceptor no longer reaches services it has no business bracketing. That
matters on the PBFT port: switching the PBFT cursor reads the head and latest
pbft block numbers, which a reflection call should not pay for.

Also scope the cursor to the synchronous handler callback rather than to "the
call" in the docs and javadoc: a call spans several listener callbacks and may
span several threads, so call-lifetime scoping is not a safe model for a
ThreadLocal. Note that the synchronous-handler property is an implementation
invariant rather than a type-level guarantee, and correct the offset comment —
only PBFT computes a head-to-pbft offset, SOLIDITY does not.

Tests: CursorInterceptorScopeTest drives interceptCall on one thread and
onHalfClose on another, so an implementation that scoped the cursor around
interceptCall fails by construction instead of by scheduling luck.
CursorInterceptorAttachmentTest pins both properties of the change above.
A handler that called responseObserver.onError(...) and then fell
through to responseObserver.onCompleted() closed the call twice; the
second close() hits checkState(!closeCalled, "call already closed") in
ServerCallImpl and throws. Clients were unaffected — onError had already
closed the call with the error status — but every failed call cost a
server-side IllegalStateException.

WalletApi already used the return form in the handlers it had been
hardened in; 24 handlers in the same file still fell through. Adds the
missing return to all of them: 17 in WalletApi and 7 shared helpers
reachable from both WalletApi and WalletSolidityApi.
Three gaps the existing suite left open:

RpcApiServiceErrorPathTest drives every unary handler of WalletApi and
WalletSolidityApi with collaborators that throw, and asserts none of
them terminates the call more than once. Reverting the return fix makes
it fail on getDelegatedResourceV2, getPendingSize and getBlock, so it
also reaches the shared *Common helpers.

CursorInterceptorWiringTest runs the real addService of the solidity and
pbft services against a mock builder and asserts both shared read
services are registered as intercepted definitions. Dropping
ServerInterceptors.intercept left every existing test green while the
port silently served HEAD; this is the grpc counterpart of
CursorFilterInstallationTest.

RpcApiServicesTest now drives getPaginatedNowWitnessList and
getTransactionInfoByBlockNum on the pbft stub as well, pinning the one
intentional behaviour change of the merge.
Three of the tests written while designing the cursor interceptor were
exploration scaffolding, not regression guards:

CursorInterceptorAttachmentTest drove probe services only, so switching
the production code back to server-level attachment left it green; the
wiring it was meant to justify is now covered by
CursorInterceptorWiringTest, and the reasoning belongs in the PR.

WalletSolidityApiMethodSubsetTest asserted every WalletSolidity method
exists on WalletApi, which the compiler enforces now that the handlers
delegate by name. It would also fail on a legitimate WalletSolidity-only
method implemented in place.

GrpcInterceptorProbeTest kept an ordering assertion whose premise no
longer holds — service-level attachment made registration order
irrelevant to the cursor services — plus two library-property tests
already covered through the production interceptor, and printed traces
left over from probing.

What that last class did earn is kept as CursorInterceptorServerTest: a
real server proving gRPC runs the handler inline from onHalfClose on the
same thread, which CursorInterceptorScopeTest cannot show on its
synthetic harness and which fails silently in production if it breaks.

Also drops two javadoc references to the deleted subset test.
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