refactor(api): remove dead WalletExtension gRPC service and config - #6975
0xbigapple wants to merge 1 commit into
Conversation
The four WalletExtension RPCs have returned UNIMPLEMENTED since 2019; the service was only registered on solidity nodes behind node.walletExtensionApi, which config.conf enabled but reference.conf disabled. Remove the service, its now-unreferenced messages (including the TimeMessage/TimePaginatedMessage orphans left by the 2018 RPC removal), the config key, and the dead client/test helpers. Log a removal warning when the old key is still present in operator configs.
| } | ||
|
|
||
| // node.walletExtensionApi (removed): the WalletExtension gRPC service no longer exists | ||
| if (section.hasPath("walletExtensionApi")) { |
There was a problem hiding this comment.
[NIT] First node.* key removed without a deprecation period — please record the exception in release notes
node.walletExtensionApi was a published config key (present in both shipped samples: config.conf defaulted it to true, reference.conf to false). The project config convention calls for a full deprecation cycle before removing a published key; this PR removes it in one step, with only this warn-and-ignore fallback. The practical impact is nil — old configs still boot (unknown keys are ignored) and operators get a clear warning, and ArgsTest.testRemovedWalletExtensionApiKeyIsIgnored locks that behavior in — so this reads as a documentable exception rather than a contract violation.
Suggestion: list the removal explicitly in the release notes ("Removed: WalletExtension gRPC service (4 RPCs) + node.walletExtensionApi config key") so the skipped deprecation cycle is visible to operators and future reviewers.
There was a problem hiding this comment.
Thanks for the suggestion. Agreed on the release note, will add it.
There was a problem hiding this comment.
[SHOULD] Please link the release-note/migration entry and the downstream follow-ups requested in #6931 before merge. The current diff contains no release note, and "Follow up" lists projects without tracking links. Cover the generated-code API break, reflection changes, the legacy-config warning, and the TronGrid alternative; link follow-ups for the affected protocol, clients, documentation, and deployment templates.
Please also update #6921's principle 2 and item 4 to record the agreed direct-removal exception and target release; its body still describes a two-stage deprecation plan.
waynercheung
left a comment
waynercheung
left a comment
There was a problem hiding this comment.
Reviewed ca08100d717a0ba5cf97fc615ef7124eb84c213e. The removal scope matches #6931, and I found no remaining code references to the removed types. Checkstyle and the 43 targeted tests in ArgsTest, ParameterTest, and UtilMockTest pass. I found no consensus/database changes or confirmed runtime correctness defect.
[SHOULD] Please narrow the "compile-time impact only / runtime behavior is unchanged" claim in the PR description and #6931. On a solidity node with WalletExtension enabled and node.disabledApi = ["gettransactionsfromthis"], the registered method returns UNAVAILABLE through RpcApiAccessInterceptor; after removal, gRPC returns UNIMPLEMENTED without running the configured access/rate-limit interceptors. Reflection output also changes, and replacing the protocol artifact can break compiled consumers that reference the removed types. Please document these intentional compatibility changes.
Requesting changes to complete the config/gRPC regression coverage and the previously requested migration documentation and follow-up links. These do not require retaining the removed service.
| * NodeConfigTest because module jacoco reports only aggregate framework execution data. | ||
| */ | ||
| @Test | ||
| public void testRemovedWalletExtensionApiKeyIsIgnored() { |
There was a problem hiding this comment.
[SHOULD] This only verifies that walletExtensionApi = true parses; deleting the warning branch would still pass. Please cover absent / true / false and assert the removal WARN, with no WalletExtension removal warning when the key is absent.
Also add the gRPC regression requested in #6931: call all four legacy method names via explicit MethodDescriptors and assert UNIMPLEMENTED. Verify service registration using RpcApiService with isSolidityNode() == true and the legacy key present: WalletExtension must be absent and WalletSolidity retained. The existing RpcApiServicesTest covers multiple ports but does not exercise this node-mode registration branch.
| } | ||
|
|
||
| // node.walletExtensionApi (removed): the WalletExtension gRPC service no longer exists | ||
| if (section.hasPath("walletExtensionApi")) { |
There was a problem hiding this comment.
[SHOULD] Please link the release-note/migration entry and the downstream follow-ups requested in #6931 before merge. The current diff contains no release note, and "Follow up" lists projects without tracking links. Cover the generated-code API break, reflection changes, the legacy-config warning, and the TronGrid alternative; link follow-ups for the affected protocol, clients, documentation, and deployment templates.
Please also update #6921's principle 2 and item 4 to record the agreed direct-removal exception and target release; its body still describes a two-stage deprecation plan.
5 participants
What does this PR do?
close #6931.
Removes the dead
WalletExtensiongRPC service and everything reachable only from it:api.proto:service WalletExtension(GetTransactionsFromThis/2,GetTransactionsToThis/2), plus messagesAccountPaginated,TransactionList,TransactionListExtention(referenced only by these four RPCs) andTimeMessage/TimePaginatedMessage(request types of the WalletExtension*ByTimestampRPCs deleted in 2018, orphaned ever since)RpcApiService: the registration branch and the emptyWalletExtensionApiinner classnode.walletExtensionApiconfig item:CommonParameter/NodeConfigfields, theArgsbinding, and the key inreference.conf/config.conf/config-shield.conf. Following the retirement convention fornode.*keys,NodeConfig.fromConfignow logs a removal warning when the old key is still present in an operator configUtil.printTransactionList(sole caller was its own mock test), the WalletExtension stub and wrappers in test utilitiesGrpcClient/WalletClient,HttpMethed.getTransactions{From,To}ThisFromSolidity(targets/walletextension/*HTTP paths that have no servlet), and commented-outgetTransactionsByTimestamp/getAssetIssueListByTimestampblocksWhy are these changes required?
WalletExtensionhas had no implementation in any release since v3.7 (2020-03):RpcApiService$WalletExtensionApioverrides none of the four RPCs, so every call falls through to the generatedImplBasedefault handlers and returnsUNIMPLEMENTED. This makesnode.walletExtensionApibehavior-irrelevant — enabled, a solidity node registers a service with zero implemented methods (advertised via gRPC reflection whennode.rpc.reflectionServiceis on); disabled, callers get the sameUNIMPLEMENTED. Removing it also resolves the default-value inconsistency betweenconfig.conf(true) andreference.conf(false).Six years of unconditional
UNIMPLEMENTEDrules out any functional dependency, so the service is removed directly without a deprecation period, following existing practice for dead interfaces.This PR has been tested by:
Follow up
Ecosystem code that still compiles against the removed stubs/messages needs a sync: the
tronprotocol/protocolmirror, the documentation site, and older wallet-cli/trident versions. Compile-time impact only — runtime behavior is unchanged (UNIMPLEMENTEDbefore and after).Extra details
None.