Consolidate RPC Client Interfaces #1323
Merged
Conversation
This file contains 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
Simplify the file name by removing the redundant package name prefix, making it more concise and consistent. This is a refactor commit and is unrelated to the other changes in this PR.
Roasbeef
reviewed
Jan 24, 2025
| @@ -58,29 +62,60 @@ func Fatal(err error) { | |||
| os.Exit(1) | |||
| } | |||
|
|
|||
| func getClient(ctx *cli.Context) (taprpc.TaprootAssetsClient, func()) { | |||
| // RpcClientsBundle is an interface that groups all the RPC clients. | |||
| type RpcClientsBundle interface { | |||
There was a problem hiding this comment.
Perhaps we should just define this in taprpc? Then that way we don't have anything importing from the cmd package, which is really just meant to be an executable.
What does the import cycle you ran into look like?
There was a problem hiding this comment.
All these proto message types would need to be moved/refactored (and related golang code):
Targets
Occurrences of 'taprpc' in Selected directories with mask '*.proto'
Found occurrences in Selected directories with mask '*.proto' (38 usages found)
Unclassified (38 usages found)
taproot-assets (38 usages found)
taprpc/assetwalletrpc (14 usages found)
assetwallet.proto (14 usages found)
34 returns (taprpc.SendAssetResponse);
52 returns (taprpc.SendAssetResponse);
205 taprpc.OutPoint outpoint = 1;
347 repeated taprpc.OutPoint lnd_locked_utxos = 6;
381 repeated taprpc.OutPoint lnd_locked_utxos = 5;
389 taprpc.KeyDescriptor internal_key = 1;
397 taprpc.ScriptKey script_key = 1;
407 taprpc.KeyDescriptor internal_key = 1;
418 taprpc.ScriptKey script_key = 1;
426 taprpc.OutPoint outpoint = 3;
451 taprpc.OutPoint outpoint = 2;
468 taprpc.OutPoint outpoint = 1;
475 taprpc.ScriptKey script_key = 1;
479 taprpc.ScriptKey script_key = 1;
taprpc/mintrpc (18 usages found)
mint.proto (18 usages found)
63 taprpc.AssetVersion asset_version = 1;
66 taprpc.AssetType asset_type = 2;
75 taprpc.AssetMeta asset_meta = 4;
102 taprpc.KeyDescriptor group_internal_key = 9;
115 taprpc.ScriptKey script_key = 11;
123 taprpc.GroupKeyRequest group_key_request = 2;
126 taprpc.GroupVirtualTx group_virtual_tx = 3;
134 taprpc.AssetVersion asset_version = 1;
137 taprpc.AssetType asset_type = 2;
146 taprpc.AssetMeta asset_meta = 4;
181 taprpc.KeyDescriptor group_internal_key = 10;
201 taprpc.ScriptKey script_key = 12;
225 taprpc.ExternalKey external_group_key = 14;
319 taprpc.TapscriptFullTree full_tree = 3;
322 taprpc.TapBranch branch = 4;
342 repeated taprpc.GroupWitness group_witnesses = 2;
376 taprpc.TapscriptFullTree full_tree = 3;
379 taprpc.TapBranch branch = 4;
taprpc/tapchannelrpc (3 usages found)
tapchannel.proto (3 usages found)
215 taprpc.DecimalDisplay decimal_display = 2;
218 taprpc.AssetGroup asset_group = 3;
222 taprpc.GenesisInfo genesis_info = 4;
taprpc/tapdevrpc (1 usage found)
tapdev.proto (1 usage found)
101 taprpc.Addr address = 2;
taprpc/universerpc (2 usages found)
universe.proto (2 usages found)
323 taprpc.Asset asset = 1;
565 taprpc.AssetType asset_type = 5;
I think we should actually do that in the future by adding an RPC package with useful TAP primitive types.
But I'm hoping to avoid making that change right now and just get to a point where we can include tapcli minting commands in our itests.
There was a problem hiding this comment.
Solution proposal: #1328
Pull Request Test Coverage Report for Build 12986915762Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Refactors the RpcBundle to RpcClientsBundle for better clarity and consistency. Also adds previously missing RPC clients to ensure all required clients are included.
To ensure consistency, re-use interface command.RpcClientsBundle in the itest rpcClient struct.
This commit removes duplicate code.
ffranr
force-pushed
the
rpc-clients-refactor
branch
from
d042438 to
c46c69e
Compare
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.
This PR introduces the following changes:
RpcBundletoRpcClientsBundlefor improved clarity and consistency.RpcClientsBundleto ensure all required clients are included.commands.RpcClientsBundleinterface in theitestrpcClientstruct for consistency.TapdClientwithcommands.RpcClientsBundlein integration tests.These changes streamline the codebase and enhance maintainability by unifying the handling of RPC clients across components.
I attempted to define
RpcClientsBundleintaprpcbut it leads to an import cycle withassetwalletrpc.