Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

litclient: add taprpc to registrations #571

Merged
merged 2 commits into from
Jun 12, 2023
Merged

litclient: add taprpc to registrations #571

merged 2 commits into from
Jun 12, 2023

Conversation

ViktorTigerstrom
Copy link
Contributor

This PR adds the taprpc to the litclient's Registrations array, to ensure that the TaprootAssets callbacks are registered and available in the litclient.

@ViktorTigerstrom ViktorTigerstrom requested review from ellemouton and jamaljsr and removed request for jamaljsr and ellemouton June 8, 2023 13:56
jamaljsr
jamaljsr approved these changes Jun 8, 2023
View reviewed changes
Copy link
Member

@jamaljsr jamaljsr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this needs a rebase, but I tested this with lightninglabs/lightning-node-connect#77 and it worked. 👌

@ellemouton
Copy link
Member

ellemouton commented Jun 8, 2023 •
edited

This will need a PR in the taproot-assets repo first. The build is currently failing cause the JSON callback method used from the TAP code is only built if the js build tag is set.

So this line will need to be edited so that it doesnt add the build_tags=// +build js part and then you can run make protos which should regen the file to not have the // +build js line

@levmi
Copy link

levmi commented Jun 8, 2023

cc @dstadulis who can help us prio any necessary change on the tapd side

@ViktorTigerstrom ViktorTigerstrom force-pushed the 2023-06-add-taprpc-to-litclient-registrations branch from 2d7daa4 to 031175f Compare June 9, 2023 08:57
@ViktorTigerstrom
Copy link
Contributor Author

Opened a PR for the required fix in the taproot-assets repo, and updated this PR to use a forked version containing the fix.

Also updated this PR to register the assetwalletrpc universerpc JSON callbacks in the litclients Registrations array as well!

@ViktorTigerstrom ViktorTigerstrom force-pushed the 2023-06-add-taprpc-to-litclient-registrations branch 3 times, most recently from a0c75c6 to 6a8f426 Compare June 11, 2023 22:13
@ViktorTigerstrom
Copy link
Contributor Author

ViktorTigerstrom commented Jun 11, 2023 •
edited

Updated this PR to use the latest version of the main branch for the taproot-asset dependency to include the required fix. Also update the litclient Registrations to include mintrpc 🚀.

Also rebased on the latest version of master.

@ellemouton
Copy link
Member

we might not want to include all the other changes in the taproot-assets code since the v0.2.0 release since it could have behaviour changes that the user is not expecting. Users would then be running an un-released version of taproot-assets if they were to run it via LiT (if we were to do another LiT release that did not include a Taproot-assets bump)

This is something that we might want to do quite often in LiT & I think we dont always want to have to wait for a release on the sub-server side just cause we want to include some changes on the code-structure level that wont actually have behaviour changes for the user. So I think we should decide on a pattern for including such changes.

My suggestion would be: we should create a v0.2.0-lit-0 tag (using taproot-assets as an example) in that repo which points to a commit that includes the released version (v0.2.0) plus any specific commits we require for Lit. In this case it would be just the two commits added here. The next time we add a commit there, we add a v0.2.0-lit-1 tag etc. Then on the next taproot-assets release, we just use the v0.2.1 tag again etc etc.

Keen to hear what @guggero things of this idea 🙏

@guggero
Copy link
Member

guggero commented Jun 12, 2023

Good point, @ellemouton. There weren't any braking changes merged in taproot-assets recently, only bug fixes. And v0.2.1 should be fairly close. But IMO it still makes sense to create a branch, so I did that and pushed the v0.2.0-lit-0 tag.

@ellemouton
Copy link
Member

shweet! thanks @guggero 🙏 🚀

@ViktorTigerstrom - can you update this PR to point to the new tag? after that, this is g2g!

ViktorTigerstrom and others added 2 commits June 12, 2023 10:38
@ViktorTigerstrom
mod: Update taproot-assets dependency
77c7229 
Update the taproot-assets dependency to a version of the main branch
that includes a commit which contains a fix that removes the js build
tag requirement when using the taprpc packages' JSON callback functions.

Also replace the google.golang.org/protobuf dependency to forked version
which is required for the taproot-assets dependency to work properly.
@ViktorTigerstrom
litclient: add taprpc packages to Registrations
f981d96 
Add the `taprpc`, `assetwalletrpc`, `mintrpc` and the `universerpc` JSON
callbacks to the litclient's `Registrations` array. This allows the
litclient to use the rpc functions contained in the JSON callbacks.
@ViktorTigerstrom ViktorTigerstrom force-pushed the 2023-06-add-taprpc-to-litclient-registrations branch from 6a8f426 to f981d96 Compare June 12, 2023 08:38
@ViktorTigerstrom
Copy link
Contributor Author

Thanks for the great feedback @ellemouton! Makes a lot sense. And thanks for creating the tag so quickly @guggero!!

Updated the dependency to use the new v0.2.0-lit-0 tag with the latest push!

guggero
guggero approved these changes Jun 12, 2023
View reviewed changes
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK, LGTM 🎉

go.mod
// We want to format raw bytes as hex instead of base64. The forked version
// allows us to specify that as an option. This is required for the
// taproot-assets dependency to function properly.
replace google.golang.org/protobuf => github.com/lightninglabs/protobuf-go-hex-display v1.30.0-hex-display
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

ellemouton
ellemouton approved these changes Jun 12, 2023
View reviewed changes
Copy link
Member

@ellemouton ellemouton left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK 🚀

@ellemouton ellemouton merged commit ee04747 into lightninglabs:master Jun 12, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guggero guggero guggero approved these changes

@jamaljsr jamaljsr jamaljsr approved these changes

@ellemouton ellemouton ellemouton approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ViktorTigerstrom @ellemouton @levmi @guggero @jamaljsr