feat(API): refactor merge http servlets - #20
Open
SeriousCoding789 wants to merge 28 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 |
Little-Peony
force-pushed
the
refactor_merge_http_servlets
branch
2 times, most recently
from
August 24, 2026 09:58
124abb6 to
04a92f7
Compare
SeriousCoding789
changed the title
…-publish-jdk8 fix(build): skip errorprone publishing on JDK 8
Replace the hand-written HttpApiDef enum with HttpApiRegistry, built at class-load from a single @httpapi(value, access, surfaces) declaration on each servlet. The four http services mount from the derived registry, so an endpoint is declared once and every surface stays in sync by construction. - @httpapi is never @inherited and is read only via getDeclaredAnnotation, so a servlet subclass can never inherit its parent's exposure. - Every concrete servlet in the package must declare @httpapi or @HttpApiExcluded; the registry fails the boot otherwise, before any Jetty bind, and also refuses to boot on an empty scan. - The API x surface x access audit matrix is generated and checked against a committed snapshot; an independent fixture of the pre-refactor hand-written routes is the parity baseline, so the derived table cannot validate itself. That baseline caught five shielded read endpoints (getmerkletreevoucherinfo, scanandmarknotebyivk, scannotebyivk, scannotebyovk, isspend) that the enum had dropped from the PBFT surface though they are live on develop; restored. Tests: HttpApiRegistryTest with negative fixtures for each invariant, plus WalletCursorFilterTest and CursorFilterInstallationTest pinning the read-cursor filter behaviour and installation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
createshieldedcontractparameters and its withoutask variant compose an unsigned transaction via wallet.createShieldedContractParameters*, so they are BUILD endpoints; they were mislabeled Access.READ.
Replace the hand-written HttpApiDef enum with HttpApiRegistry, built at class-load from a single @httpapi(value, access, surfaces) declaration on each servlet. The four http services mount from the derived registry, so an endpoint is declared once and every surface stays in sync by construction. - @httpapi is never @inherited and is read only via getDeclaredAnnotation, so a servlet subclass can never inherit its parent's exposure. - Every concrete servlet in the package must declare @httpapi or @HttpApiExcluded; the registry fails the boot otherwise, before any Jetty bind, and also refuses to boot on an empty scan. - The API x surface x access audit matrix is generated and checked against a committed snapshot; an independent fixture of the pre-refactor hand-written routes is the parity baseline, so the derived table cannot validate itself. Five sapling shielded note-scan endpoints (getmerkletreevoucherinfo, scanandmarknotebyivk, scannotebyivk, scannotebyovk, isspend) were live on develop's PBFT port only while disabled on every other surface; they stay disabled here too (@HttpApiExcluded), aligning all four surfaces. The parity test records this as an intentional PBFT removal vs the develop baseline. Tests: HttpApiRegistryTest with negative fixtures for each invariant, plus WalletCursorFilterTest and CursorFilterInstallationTest pinning the read-cursor filter behaviour and installation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Migration correctness is verified by diffing against upstream, so the repo no longer carries the frozen fixtures: remove pre-refactor-routes.txt and api-audit-matrix.txt, their two tests in HttpApiRegistryTest, and the now unused HttpApiRegistry.auditMatrix(). The rule-based invariant tests (non-READ is FULL-only, @inherited guard, completeness net, suffix and surface rejections) stay. Also adds nested / whitespace / wildcard probe fixtures under regtest.
An adversarial review of HttpApiRegistry found four ways a mistaken annotation could reach a running node instead of failing the boot. - Suffix syntax. The suffix is concatenated into a jetty path spec, but only '/' and blank were rejected. "*" passed validation and mounted the servlet as the prefix wildcard /wallet/*, swallowing every sibling endpoint and, because a wildcard match rewrites getServletPath(), slipping past the filters that key off it; a suffix carrying whitespace was checked with trim() but stored raw, mounting an endpoint at a path no client can request. Both are now rejected by a single path-token rule [A-Za-z0-9_.-]+, which all 120 live endpoints already satisfy (four of them are camelCase, so the rule is not lowercase-only). - Unmountable endpoint declarations. A servlet that is abstract, or nested inside another class, was dropped before its annotations were read, so an @httpapi on one produced no endpoint and no error — the silent omission this registry exists to prevent, and invisible to the completeness test because it iterated the same filtered list. Such classes are now scanned and must declare neither annotation. - Spring stereotypes. The @component check only accepted the literal annotation, so a servlet marked with a meta-annotated stereotype would have aborted the boot despite being a valid bean. It now uses get semantics (direct + meta, never inherited from a superclass). - Boot failure path. HttpService.start() calls addServlet() synchronously, so a registry failure escaped ServiceContainer's TronError wrapper and never reached the logged System.exit. Registry failures are now raised as TronError(API_SERVER_INIT); validation itself still throws IllegalStateException so it stays unit-testable. Adds negative tests for the wildcard, whitespace and nested cases, and asserts every live suffix satisfies the path-token rule.
These two assemble ShieldedTRC20Parameters for a caller to put into a transaction, so they are BUILD rather than READ. An earlier commit had already corrected this; the annotation refactor reverted it while aligning access values against the old HttpApiDef enum, which was the source of the mistake.
Little-Peony
force-pushed
the
refactor_merge_http_servlets
branch
from
September 1, 2026 03:08
4c26109 to
9e23145
Compare
HttpApiRegistry validates the whole table when it is first touched, so six of the tests could not fail on their own: any violation aborts the static initializer and takes every other test with it. Removed: the non-empty smoke test, the @inherited guard (buildFromPackage asserts it in production code), and the four sweeps over the live registry for FULL-only access, @component, exactly-one-annotation and suffix syntax — all four re-check what validate() throws on while the table is being built. The rejection tests keep the real coverage: each drives one validation branch through a fixture package and asserts the boot failure message. The four mount-parity tests, which pin that each service mounts exactly the registry's set for its surface, are untouched. all(), Entry#getAccess() and scanConcreteServlets() had no caller left outside those tests, so the registry drops them and Entry stops carrying an access field nothing reads. Also drops WalletCursorFilterTest#testCursorIsResetOnEverySequential- Request: calling a stateless filter twice adds nothing over the single set-then-reset ordering test next to it.
…ched Jetty's ConnectionLimit suspends accept() on every connector when the cap is hit and never re-enables it while slow clients hold their sockets, so 50 idle connections per port block all HTTP clients. Its own throttle branch only runs when an idle timeout is set; set 5s so existing connections drain and the acceptor resumes. Normal traffic is unaffected: the value applies only while over the limit and Jetty restores the connector default afterwards.
… set Gate the pbft variant of gettransactioninfobyblocknum, which was mounted on the pbft surface without a matching filterPaths entry while its full and solidity variants were already gated. Drop the five sapling shielded note-scan suffixes on all three surfaces: their servlets are @HttpApiExcluded and the paths are no longer mounted anywhere, so the entries were dead configuration. Every remaining entry now corresponds to a mounted endpoint, and every gated suffix is gated on each surface that mounts it.
warku123
reviewed
Sep 2, 2026
|
|
||
| @Component | ||
| @Slf4j(topic = "API") | ||
| @HttpApi(value = "getaccount", access = Access.READ, |
There was a problem hiding this comment.
[MUST] The rate limiter is keyed by the servlet class name, so all surfaces now share one limiter instance.
Before this PR each surface had its own wrapper servlet class (e.g. GetAccountOnSolidityServlet, GetAccountOnPBFTServlet), so the http_<class> keys were distinct and every port had its own permit pool. After the merge, a single GetAccountServlet bean is mounted on /wallet, /walletsolidity and /walletpbft — the same simple name resolves to the same RateLimiterContainer entry, giving one shared QPS budget across three ports. Total capacity drops from 3×qps to 1×qps.
Also, existing rate.limiter.httpMap entries referencing the old *OnSolidity / *OnPBFT class names no longer match anything and are silently ignored (addRateContainer falls back to the default adapter).
Suggestion: key the limiter by surface + suffix (or request context path), and/or document the httpMap key migration in the PR.
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
5 participants
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#6922.
developserves the four HTTP surfaces (FULL, SOLIDITY, PBFT, and the standalone SolidityNode) with parallel sets of servlets and four hand-written registration lists. This PR collapses them onto one servlet per endpoint:*OnSolidityServlet,*OnPBFTServlet,http/solidity/*SolidityServlet). 98 of them carry no logic at all — the whole class body iswalletOnSolidity.futureGet(() -> super.doGet(req, resp)), i.e. a class per endpoint whose only job is to switch the read cursor.WalletCursorFilter+SolidityCursorFilter/PbftCursorFilter) mounted on/*of the solidity and pbft ports. Switching the cursor is a per-request, thread-level concern; it does not need a subclass per endpoint.@HttpApi/@HttpApiExcludeddeclared on the servlet itself, and derives a read-onlyHttpApiRegistryfrom them by classpath scan. Adding an endpoint becomes a one-place change instead of a four-place change.READendpoint on a cursor surface, a servlet declaring neither annotation or both, an endpoint declared on a nested or abstract class, and a missing@Componentall fail the node withTronError(API_SERVER_INIT)instead of silently dropping an endpoint.356 files changed, +2136 / −4751.
Why are these changes required?
Duplicating an endpoint across surfaces is not free — it drifts silently, and two live examples on
developcame out of this work:gettransactioninfobyidnever picked up thevisible=true→convertLogAddressToTronAddressstep the base servlet has, so it returnslog[].addressin hex where FullNode returns base58.Both are "change one place, forget the other" bugs. With one servlet per endpoint and a derived registry, a surface can no longer fall behind on its own.
Behaviour differences vs
developEvery per-surface servlet was classified by whether its body contains
futureGet: 98 pure cursor delegations (cannot drift) and 2 hand-copied implementations (can). Per-surface result:Client-visible changes, all deliberate and worth a release note:
/walletsolidity/gettransactioninfobyid— withvisible=trueon a transaction that has logs,log[].addresschanges from hex to Tron base58, matching FullNode. This is the drift fix above;visible=falseand log-free transactions are unaffected.getmerkletreevoucherinfo,isspend,scanandmarknotebyivk,scannotebyivk,scannotebyovknow return 404 on/walletpbft/*. They were disabled on every other surface in 2020; PBFT is catching up, not regressing.getpaginatednowwitnesslistandgettransactioninfobyblocknum, which FULL / SOLIDITY / SolidityNode already expose. Pure addition.Error responses on the two former SolidityNode copies now go through
Util.processError(the standard{"Error": ...}body) instead of writing the raw exception message into the response.This PR has been tested by:
Unit Tests — 24 new tests, all passing:
HttpApiRegistryTest(17). Twelve drive one validation branch each through a fixture package underhttp/regtest/*and assert the boot failure it produces: a servlet declaring neither annotation or both, an endpoint on a nested or abstract class, a duplicate(surface, suffix), a blank suffix, a/in a suffix, a*or whitespace suffix, a missing@Component, an empty surface list, and a non-READendpoint on a cursor surface. One builds a valid fixture package. The remaining four are the mount-parity tests: each service's mounted path set equals the registry's derived set for its surface, so an endpoint cannot be declared and left unmounted, or mounted without being declared.CursorFilterInstallationTest(4) runs each service's realaddFilterand asserts solidity and pbft install exactly one cursor filter on/*, while the FULL port and the standalone SolidityNode install none — a cursor filter on either of those would take the port off HEAD.WalletCursorFilterTest(3) asserts the cursor is set beforechain.doFilterand reset infinallyincluding when the servlet throws, and that the PBFT subclass switches to the PBFT cursor rather than SOLIDITY.There is no frozen route snapshot: the registry is validated in full when the class is first touched — before Jetty binds, and for every surface whether or not the node enables it — so the invariants are enforced at boot and the tests drive the failure paths rather than re-asserting them over the live table.
Manual Testing — brought up a private chain and checked the mounted endpoint set on every port, including that endpoints marked
@HttpApiExcludedare unreachable.Follow up
Extra details
@HttpApi/@HttpApiExcludedare deliberately not@Inherited, and the registry reads them withgetDeclaredAnnotationonly. Inheritable exposure is exactly what produced the 100 wrapper classes this PR removes — a subclass must never silently inherit its parent's surface set.