taprpc+multi: concert `taprpc` into a versioned Go module #1090

bhandras · 2024-08-16T20:20:29Z

With this PR, taprpc can now be used in other projects without also pulling in the entire taproot-assets repository as a dependency. This modularization is desirable because it reduces unnecessary dependencies, leading to cleaner and more efficient codebases. Additionally, it allows for more focused and manageable updates, improves build times, and enhances the reusability of taprpc in various contexts.

coveralls · 2024-08-16T20:34:16Z

Pull Request Test Coverage Report for Build 10425996277

Details

26 of 165 (15.76%) changed or added relevant lines in 6 files are covered.
17 unchanged lines in 6 files lost coverage.
Overall coverage decreased (-0.3%) to 39.847%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
cmd/tapcli/addrs.go	0	1	0.0%
itest/utils.go	0	1	0.0%
itest/multisig.go	0	3	0.0%
proof/courier.go	0	5	0.0%
rpcserver.go	0	49	0.0%
rpcutils/marshal.go	26	106	24.53%

Files with Coverage Reduction	New Missed Lines	%
commitment/tap.go	1	83.64%
rpcserver.go	1	0.0%
tappsbt/create.go	2	53.85%
asset/asset.go	2	81.54%
tapdb/universe.go	4	80.91%
tapdb/multiverse.go	7	60.32%

Totals
Change from base Build 10425783374:	-0.3%
Covered Lines:	23714
Relevant Lines:	59513

💛 - Coveralls

This commit transfers the marshal functions in marshal.go to a newly created rpcutils package. This preliminary change is necessary to facilitate the conversion of taprpc into a versioned Go module.

With this commit taprpc is converted to a separate go module.

ffranr

I'm uncertain about this change, as it seems primarily targeted at Go developers. However, won't the users of those endpoints still require the marshal/unmarshal functions that this PR separates from the RPC endpoint definitions? I suspect they will still need to use them, which could undermine the intended benefits of this change.

I also appreciate the current scoping we have. For example, the existing oraclerpc.IsAssetBtc is more contextually clear compared to something like rpcutils.IsAssetBtc. Simply moving the code might not be sufficient, as we lose valuable context when shifting to a more generic scope like rpcutils. I think we need to consider additional steps to preserve that context.

guggero

I see why we need to move out the marshal functions from the taprpc sub package, since they depend on other packages in taproot-assets, which would create a super awkward circular module dependency once we turn things into a module.

But maybe we don't need to move everything, just the stuff that does generic marshaling/unmarshaling? To address some of @ffranr's concerns?

guggero · 2024-08-19T12:43:51Z

go.mod

@@ -207,3 +206,6 @@ require (
 // We want to format raw bytes as hex instead of base64. The forked version
 // allows us to specify that as an option.
 replace google.golang.org/protobuf => github.com/lightninglabs/protobuf-go-hex-display v1.30.0-hex-display
+
+// Note this is a temproary replace and will be removd when taprpc is tagged.
+replace github.com/lightninglabs/taproot-assets/taprpc => ./taprpc


Hmm, I guess a go.work file wouldn't really help us here, at least not much. It would allow us to not have this replace directive. But any external project depending on this one would still need to pull the git hash/version from here, so we still need to push a tag and update the go.mod file regularly...

This makes everything quite cumbersome, especially in lightning-terminal... But I don't really have a better solution...

guggero · 2024-08-19T12:47:28Z

rpcserver.go

 	out.ProofDeliveryComplete.WhenSome(func(complete bool) {
 		if complete {
-			proofDeliveryStatus = taprpc.ProofDeliveryStatusComplete
+			proofDeliveryStatus = rpcutils.ProofDeliveryStatusComplete //nolint: lll


nit: add nolint comment before if so it counts for both branches and doesn't make line even longer?

guggero · 2024-08-19T12:52:29Z

rpcutils/marshal.go

+
+// IsAssetBtc is a helper function that returns true if the given asset
+// specifier represents BTC, and false otherwise.
+func IsAssetBtc(assetSpecifier *priceoraclerpc.AssetSpecifier) bool {


Since this method doesn't actually pull in any other taproot-assets packages, I think we can keep it where it is to address @ffranr's concerns re naming/context.

guggero · 2024-08-19T12:53:48Z

cmd/tapcli/addrs.go

@@ -82,7 +83,7 @@ func newAddr(ctx *cli.Context) error {
 	client, cleanUp := getClient(ctx)
 	defer cleanUp()

-	assetVersion, err := taprpc.MarshalAssetVersion(
+	assetVersion, err := rpcutils.MarshalAssetVersion(


Maybe we can call this package rpcmarshal instead, so it's more clear only marshaling functions (but from any sub RPC package) should go here.

bhandras · 2024-08-19T12:58:02Z

I'm uncertain about this change, as it seems primarily targeted at Go developers. However, won't the users of those endpoints still require the marshal/unmarshal functions that this PR separates from the RPC endpoint definitions? I suspect they will still need to use them, which could undermine the intended benefits of this change.

My primary goal was to reduce dependencies in the lightning-terminal repository, ensuring we rely solely on protobuf-generated Go code. I believe this approach could benefit many projects. Additionally, this effort is part of a broader initiative aimed at ensuring the lightning-terminal repository only depends on versioned RPC modules.

I also appreciate the current scoping we have. For example, the existing oraclerpc.IsAssetBtc is more contextually clear compared to something like rpcutils.IsAssetBtc. Simply moving the code might not be sufficient, as we lose valuable context when shifting to a more generic scope like rpcutils. I think we need to consider additional steps to preserve that context.

I believe scoping could be addressed separately. Better naming of the functions might resolve the issue, or alternatively, we could add scoping within the rpcutils package.

bhandras · 2024-08-19T13:00:38Z

I see why we need to move out the marshal functions from the taprpc sub package, since they depend on other packages in taproot-assets, which would create a super awkward circular module dependency once we turn things into a module.

Yes, unfortunately taprpc can't be converted into a module without resolving these circular dependencies.

But maybe we don't need to move everything, just the stuff that does generic marshaling/unmarshaling? To address some of @ffranr's concerns?

Would that address your concerns @ffranr ?

ffranr · 2024-08-19T13:31:50Z

My primary goal was to reduce dependencies in the lightning-terminal repository, ensuring we rely solely on protobuf-generated Go code. I believe this approach could benefit many projects. Additionally, this effort is part of a broader initiative aimed at ensuring the lightning-terminal repository only depends on versioned RPC modules.

I don't see the rationale for why litd should depend exclusively on protobuf-generated Go code. These are the questions I've asked myself, and I think the answer is negative on all counts:

Does it reduce the size of the litd release binary?
Will the tap dependency in litd need to be updated less often?
Will it simplify litd dev work?
Will litd users see some benefit?
Is this going to simplify the tap codebase?

Please help me see what I'm missing here. I would have thought that the compiler could drop any unused code, maybe not? Maybe it does simplify litd dev work somehow?

ensuring the lightning-terminal repository only depends on versioned RPC modules

But doesn't litd also need to use the (un)marshalling functionality?

bhandras · 2024-08-19T16:17:49Z

My primary goal was to reduce dependencies in the lightning-terminal repository, ensuring we rely solely on protobuf-generated Go code. I believe this approach could benefit many projects. Additionally, this effort is part of a broader initiative aimed at ensuring the lightning-terminal repository only depends on versioned RPC modules.

I don't see the rationale for why litd should depend exclusively on protobuf-generated Go code. These are the questions I've asked myself, and I think the answer is negative on all counts:

Does it reduce the size of the litd release binary?

Will the tap dependency in litd need to be updated less often?

Will it simplify litd dev work?

Will litd users see some benefit?

Is this going to simplify the tap codebase?

Please help me see what I'm missing here. I would have thought that the compiler could drop any unused code, maybe not? Maybe it does simplify litd dev work somehow?

ensuring the lightning-terminal repository only depends on versioned RPC modules

But doesn't litd also need to use the (un)marshalling functionality?

I think these are all great questions and from litd's point of view as it embeds other projects versioned RPC is not super important, however users of litd can easily run into unresolvable dependency issues (when depending on different versions of the packages litd depends on). Let me give you a concrete example:

I'd like to create an integration testing framework where I run litd binaries and interact through litrpc with those binaries (provided that litrpc is a module to avoid unresolvable dependencies) in order to be able to test with asset channels. I'd also not like to pull the whole tapd as a dependency to further reduce any potential compilation complexities. For this specific use case I just need a litd binary and clean dependency on litrpc and taprpc.

jharveyb · 2024-08-19T22:27:33Z

taprpc/go.mod

+	modernc.org/strutil v1.2.0 // indirect
+	modernc.org/token v1.1.0 // indirect
+	sigs.k8s.io/yaml v1.2.0 // indirect
+)


Does this need the existing replace that was being used for the tap project?

replace google.golang.org/protobuf => github.com/lightninglabs/protobuf-go-hex-display v1.30.0-hex-display

No, we only need that in the main module where we actually use the code from the forked version.

jharveyb · 2024-10-29T20:10:19Z

Noting that this is also an issue that also affects users building in other languages.

lightninglabs-deploy · 2024-11-05T21:04:22Z

@bhandras, remember to re-request review from reviewers when ready

bhandras force-pushed the taprpc-module branch from 60fda5e to 391c465 Compare August 16, 2024 20:25

bhandras mentioned this pull request Aug 16, 2024

RPC Subpackages as Tagged Modules lightninglabs/lightning-terminal#821

Closed

bhandras force-pushed the taprpc-module branch from 4cc2f05 to 3371bdf Compare August 16, 2024 20:37

bhandras added 4 commits August 16, 2024 22:40

taprpc+multi: relocate marshal functions from taprpc package

dd1ecc4

This commit transfers the marshal functions in marshal.go to a newly created rpcutils package. This preliminary change is necessary to facilitate the conversion of taprpc into a versioned Go module.

taprpc+mutli: move marshal functions from priceoraclerpc and universerpc

ae0f2c6

taprpc: convert taprpc to a Go module

051b672

With this commit taprpc is converted to a separate go module.

docs: fix basic-price-oracle

596791e

bhandras force-pushed the taprpc-module branch from 3371bdf to 596791e Compare August 16, 2024 20:40

bhandras requested review from guggero and ffranr August 16, 2024 20:48

ffranr reviewed Aug 19, 2024

View reviewed changes

guggero reviewed Aug 19, 2024

View reviewed changes

dstadulis assigned bhandras Aug 19, 2024

dstadulis added gRPC CI build labels Aug 19, 2024

jharveyb reviewed Aug 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

taprpc+multi: concert `taprpc` into a versioned Go module #1090

taprpc+multi: concert `taprpc` into a versioned Go module #1090

bhandras commented Aug 16, 2024

coveralls commented Aug 16, 2024 •

edited

Loading

ffranr left a comment

guggero left a comment

guggero Aug 19, 2024

guggero Aug 19, 2024

guggero Aug 19, 2024

guggero Aug 19, 2024

bhandras commented Aug 19, 2024

bhandras commented Aug 19, 2024

ffranr commented Aug 19, 2024 •

edited

Loading

bhandras commented Aug 19, 2024

jharveyb Aug 19, 2024

guggero Aug 20, 2024

jharveyb commented Oct 29, 2024

lightninglabs-deploy commented Nov 5, 2024

taprpc+multi: concert taprpc into a versioned Go module #1090

Are you sure you want to change the base?

taprpc+multi: concert taprpc into a versioned Go module #1090

Conversation

bhandras commented Aug 16, 2024

coveralls commented Aug 16, 2024 • edited Loading

Pull Request Test Coverage Report for Build 10425996277

Details

💛 - Coveralls

ffranr left a comment

Choose a reason for hiding this comment

guggero left a comment

Choose a reason for hiding this comment

guggero Aug 19, 2024

Choose a reason for hiding this comment

guggero Aug 19, 2024

Choose a reason for hiding this comment

guggero Aug 19, 2024

Choose a reason for hiding this comment

guggero Aug 19, 2024

Choose a reason for hiding this comment

bhandras commented Aug 19, 2024

bhandras commented Aug 19, 2024

ffranr commented Aug 19, 2024 • edited Loading

bhandras commented Aug 19, 2024

jharveyb Aug 19, 2024

Choose a reason for hiding this comment

guggero Aug 20, 2024

Choose a reason for hiding this comment

jharveyb commented Oct 29, 2024

lightninglabs-deploy commented Nov 5, 2024

taprpc+multi: concert `taprpc` into a versioned Go module #1090

taprpc+multi: concert `taprpc` into a versioned Go module #1090

coveralls commented Aug 16, 2024 •

edited

Loading

ffranr commented Aug 19, 2024 •

edited

Loading