multi: replace occurrences of "LSAT" to "L402" #730
Merged
Conversation
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
hieblmi
approved these changes
Apr 18, 2024
|
On another note, we should check if that these changes don't break the server. |
guggero
reviewed
Apr 22, 2024
looprpc/Dockerfile
Outdated
| @@ -1,4 +1,4 @@ | |||
| FROM golang:1.16.3-buster | |||
| FROM golang:1.19.10-buster | |||
There was a problem hiding this comment.
Here's an example for the changes required to the protobuf-compiler and clang-format versions if you wanted to go to a -bookworm (Go 1.2x) version: https://github.com/lightninglabs/taproot-assets/blob/main/taprpc/Dockerfile#L1
It's not too involved, so I would suggest doing it.
go.mod
Outdated
| @@ -204,4 +204,7 @@ replace google.golang.org/protobuf => github.com/lightninglabs/protobuf-go-hex-d | |||
|
|
|||
| replace github.com/lightninglabs/loop/swapserverrpc => ./swapserverrpc | |||
|
|
|||
| // Temporary added until https://github.com/lightninglabs/aperture/pull/135 is merged. | |||
| replace github.com/lightninglabs/aperture => github.com/starius/aperture v0.3.2-rc1-beta | |||
There was a problem hiding this comment.
Reminder to not merge this PR until the dependent PR was merged.
This is needed to update protobuf. Version 1.33 breaks in Go 1.16 with the following error: "//go:build comment without // +build comment". Distribution was updated from buster (10) to bookworm (12). protoc was updated from v3.6.1 to v3.21.12.
Fix the build: go mod tidy sed 's@\<lsat\>@l402@g' -i `git grep -l -w aperture/lsat` make rpc
git mv ./cmd/loop/lsat.go ./cmd/loop/l402.go sed 's@lsat@l402@g' -i `git grep -l lsat` sed 's@Lsat@L402@g' -i `git grep -l Lsat` sed 's@LSAT@L402@g' -i `git grep -l LSAT` make rpc Updated release_notes.md.
The flags were re-added in hidden mode so that users who specified them could start the daemon after upgrading. If a flag is used, a deprecation warning is printed. Updated release_notes.md.
Provide backward compatibility for clients using this API endpoint.
guggero
reviewed
Apr 25, 2024
There was a problem hiding this comment.
Code looks great now, thanks for the updates!
A remaining question around backward compatibility of the RPC interface. Normally we don't break backward compatibility for at least two major releases (at least that's the idea in lnd but in practice we end up rarely removing/breaking anything at all)...
| @@ -1,21 +1,25 @@ | |||
| FROM golang:1.16.3-buster | |||
| FROM golang:1.21.9-bookworm | |||
There was a problem hiding this comment.
Reminder to push a new tag for the swapserverrpc module after merging this PR.
| @@ -1,6 +1,6 @@ | |||
| // Code generated by protoc-gen-go. DO NOT EDIT. | |||
| // versions: | |||
| // protoc-gen-go v1.31.0 | |||
| // protoc-gen-go v1.33.0 | |||
There was a problem hiding this comment.
micro-nit (non-blocking): This should probably go into the previous commit.
There was a problem hiding this comment.
This is actually updated in this commit.
starius
added a commit
to starius/loop
that referenced
this pull request
Apr 25, 2024
This is needed not to break existing client binaries, e.g. `loop listauth`, Terminal Web, RTL. The API should be removed in a couple of releases. For now, GetLsatTokens just prints a warning message about the API being deprecated and that the client binary should be updated, and calls GetL402Tokens API, as a wrapper. Type LsatToken used by GetLsatTokens in the past was renamed to L402Token, but this does not affect binary encoding, so type L402Token can be used (as part of TokensResponse) without breaking backward compatibility. Updated release_notes.md. See lightninglabs#730 (comment)
This is needed not to break existing client binaries, e.g. `loop listauth`, Terminal Web, RTL. The API should be removed in a couple of releases. For now, GetLsatTokens just prints a warning message about the API being deprecated and that the client binary should be updated, and calls GetL402Tokens API, as a wrapper. Type LsatToken used by GetLsatTokens in the past was renamed to L402Token, but this does not affect binary encoding, so type L402Token can be used (as part of TokensResponse) without breaking backward compatibility. Updated release_notes.md. See lightninglabs#730 (comment)
hieblmi
pushed a commit
to hieblmi/loop
that referenced
this pull request
Apr 29, 2024
This is needed not to break existing client binaries, e.g. `loop listauth`, Terminal Web, RTL. The API should be removed in a couple of releases. For now, GetLsatTokens just prints a warning message about the API being deprecated and that the client binary should be updated, and calls GetL402Tokens API, as a wrapper. Type LsatToken used by GetLsatTokens in the past was renamed to L402Token, but this does not affect binary encoding, so type L402Token can be used (as part of TokensResponse) without breaking backward compatibility. Updated release_notes.md. See lightninglabs#730 (comment)
hieblmi
pushed a commit
to hieblmi/loop
that referenced
this pull request
May 6, 2024
This is needed not to break existing client binaries, e.g. `loop listauth`, Terminal Web, RTL. The API should be removed in a couple of releases. For now, GetLsatTokens just prints a warning message about the API being deprecated and that the client binary should be updated, and calls GetL402Tokens API, as a wrapper. Type LsatToken used by GetLsatTokens in the past was renamed to L402Token, but this does not affect binary encoding, so type L402Token can be used (as part of TokensResponse) without breaking backward compatibility. Updated release_notes.md. See lightninglabs#730 (comment)
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.
Description
Aperture was updated to include lightninglabs/aperture#135 . Occurrences of "LSAT" in the loop's code were replaced with "L402".
This PR depends on lightninglabs/aperture#135 . Commit "update aperture to include lsat to l402 renaming" should be updated after lightninglabs/aperture#135 is merged.
Some unrelated changes:
Pull Request Checklist
release_notes.mdif your PR contains major features, breaking changes or bugfixesThis is a breaking change!
In loopd.conf file
maxlsatcostandmaxlsatfeewere renamed tomaxl402costandmaxl402feeaccordingly. If they have been changed locally, the file has to be updated for loopd to recognize the options.The path in looprpc "/v1/lsat/tokens" was renamed to "/v1/l402/tokens" and the corresponding method was renamed from
GetLsatTokenstoGetL402Tokens. Updateloopandloopdsimultaneously otherwise this RPC won't work.