feat(API): refactor merge rpc - #17
Open
SeriousCoding789 wants to merge 16 commits into
Open
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 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 |
SeriousCoding789
changed the base branch from
feat/refactor_merge_http_servlets
to
develop
August 24, 2026 06:02
Little-Peony
force-pushed
the
refactor_merge_rpc
branch
2 times, most recently
from
September 1, 2026 03:09
6d6034d to
8b15128
Compare
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.
Little-Peony
force-pushed
the
refactor_merge_rpc
branch
from
September 1, 2026 06:39
8b15128 to
224cfc6
Compare
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Implements tronprotocol#6927 — the gRPC counterpart of the HTTP servlet dedup in tronprotocol#6922.
RpcApiServiceOnSolidityandRpcApiServiceOnPBFTeach re-declare the whole read surface as per-method delegations whose only job is to switch the per-thread read cursor: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:CursorServerInterceptorwithSolidityCursorInterceptor/PbftCursorInterceptor. It bracketsListener.onHalfClose()— the callback gRPC runs a unary handler inline from — setting the cursor before and restoring it in afinally.DatabaseApi/WalletSolidityApisingletons directly, wrapped inServerInterceptors.intercept(...).RpcApiServiceOnSoliditygoes from 488 lines to 36,RpcApiServiceOnPBFTfrom 495 to 36.RpcApiServiceitself:WalletSolidityApi(servingprotocol.WalletSolidity) re-implements read handlersWalletApi(servingprotocol.Wallet) already has. 41 of its 47 methods now delegate to the sharedWalletApisingleton; the remaining 6 already routed through shared*Common/callContracthelpers, so no duplicated handler body is left. Java is single-inheritance and gRPC generates oneImplBaseper proto service, so both classes have to stay — only the bodies go.responseObserver.onError(...)and then falls through toresponseObserver.onCompleted()closes the call twice; the secondclose()hitscheckState(!closeCalled, "call already closed")in gRPC'sServerCallImpland throws.RpcApiServicehad 32 handlers shaped that way ondevelop— 8 disappear with the duplicatedWalletSolidityApibodies, and the remaining 24 get an explicitreturn, 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 ondevelopand 18 differ. Of those 18:WalletApi— it gained areturnafterresponseObserver.onError(...)andWalletSolidityApidid not, so on the error path the copy serving the Solidity and PBFT ports falls through toonCompleted()and terminates the call twice (getMerkleTreeVoucherInfo,isSpend,scanAndMarkNoteByIvk,scanNoteByIvk,scanNoteByOvk,isShieldedTRC20ContractNoteSpent,scanShieldedTRC20NotesByIvk,scanShieldedTRC20NotesByOvk). The last two additionally have aBadItemException | ZksnarkExceptionbranch with logging that onlyWalletApiever received.getBlockByNum,getBlockByNum2) drifted the other way:WalletSolidityApihas anum >= 0guardWalletApilacks.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
returnhardening was never finished onWalletApieither — 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
developMethod sets, per port. The base
WalletSolidityApi(47 methods) andDatabaseApi(4) expose exactly the same methods before and after — only bodies changed.RpcApiService)The PBFT port gains
getPaginatedNowWitnessListandgetTransactionInfoByBlockNum— the only two methodsRpcApiServiceOnPBFTnever mirrored fromRpcApiServiceOnSolidity; they returnedUNIMPLEMENTEDthere before. Both are ordinary reads and resolve against the PBFT snapshot like every other read on that port.Handler bodies. Only three
WalletApibodies changed beyond the addedreturns:getBlockByNum/getBlockByNum2adopt the solidity copy'snum >= 0guard. No response change —Wallet#getBlockByNumalready catches theStoreExceptionand returnsnullfor a negative number, so both paths reachonNext(null); the guard only skips a futile store lookup and its log line.getAssetIssueByNamedrops the"FullNode "prefix from onelogger.debugline, which was the only difference between the two copies.Error paths. Every handler that used to emit
onErrorfollowed byonCompletednow emits a single terminal event, on all three ports. Not visible to clients —onErrorhad already closed the call with the error status and the secondclose()threw before sending anything; what goes away is one server-sideIllegalStateExceptionper failed call. Two groups:WalletischeckAllowShieldedTransactionApi(), andnode.allowShieldedTransactionApidefaults tofalse, so on a default node every such call took the double-close path.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#setCursorstill computes theheadNum - pbftNumoffset for PBFT, exactly asWalletOnPBFT.futureGetdid.Scope. gRPC only.
WalletOnCursor/WalletOnSolidity/WalletOnPBFTstay, because the HTTP and JSON-RPC servlets still callfutureGet; 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:
CursorInterceptorScopeTest(2) drivesinterceptCall()on one thread and the returned listener'sonHalfClose()on another — which is what gRPC'sSerializingExecutoris free to do. An implementation that scoped the cursor aroundinterceptCallfails here by construction rather than by luck. Also covers thefinallyreset 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 fromonHalfClose, 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 realaddServiceof both cursor services against a mock builder and asserts each shared read service is registered as an intercepted definition. DroppingServerInterceptors.interceptleaves every other test green while the port silently serves HEAD, so this is the gRPC counterpart ofCursorFilterInstallationTest.RpcApiServiceErrorPathTest(2) drives every unary handler ofWalletApiandWalletSolidityApiwith collaborators that throw, and asserts none of them terminates the call more than once. Reverting the addedreturns makes it fail ongetDelegatedResourceV2,getPendingSizeandgetBlock, so it reaches the shared*Commonhelpers as well.RpcApiServicesTest— the existing suite drives all three ports end to end over 129 tests; it now also callsgetPaginatedNowWitnessListandgetTransactionInfoByBlockNumon the PBFT stub, pinning the one intentional behaviour change.