-
Notifications
You must be signed in to change notification settings - Fork 59
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.
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
multi: change LSAT name to L402 #135
Conversation
auth: LsatAuthenticator -> L402Authenticator sed -i 's/LsatAuthenticator/L402Authenticator/g' aperture.go auth/authenticator.go auth/authenticator_test.go rename package lsat to l402 git mv lsat/ l402 sed 's@aperture/lsat@aperture/l402@g' -i `git grep -l aperture/lsat` sed -i 's@package lsat@package l402@' `git grep -l 'package lsat'` sed -i 's@lsat\.@l402.@g' -i `git grep -l 'lsat\.'` sed 's@l402.Id@lsat.Id@' -i mint/mint_test.go replace lsat with l402 in the code sed 's@lsat@l402@' -i mint/mint_test.go sed 's@Lsat@L402@' -i l402/client_interceptor.go sed 's@lsatstore@l402store@' -i l402/store_test.go replace LSAT to L402 in comments sed '/\/\//s@LSAT@L402@g' -i `git grep -l '//.*LSAT'` replace LSAT -> L402 in the code, skip when a string starts with it sed 's@\([^"/]\)LSAT@\1L402@g' -i `git grep -l LSAT`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested backward compatibility with old Loop client.
Looks good, but main change request is around existing token files for existing users/clients.
@@ -15,19 +15,19 @@ var ( | |||
|
|||
// storeFileName is the name of the file where we store the final, | |||
// valid, token to. | |||
storeFileName = "lsat.token" | |||
storeFileName = "l402.token" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to rename this, then I think we'd also need a migration. Otherwise every node that has ever paid for a Loop or Pool token will need to buy a new one (and any discounts offered to customers identified by their token ID will be lost).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch! I added migration in a separate commit (l402: migration of lsat.token* files
).
base64.StdEncoding.EncodeToString(macBytes), paymentRequest) | ||
header := r.Header | ||
header.Set("WWW-Authenticate", str) | ||
// Old loop software (via ClientInterceptor code of aperture) looks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: always add newline before comment block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@@ -71,6 +71,11 @@ func (l *L402Authenticator) Accept(header *http.Header, serviceName string) bool | |||
return true | |||
} | |||
|
|||
const ( | |||
lsatAuthScheme = "LSAT" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: some Godoc comments explaining the difference of these two would be useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed:
const (
// Outdated RFC 7235 auth-scheme used by aperture.
lsatAuthScheme = "LSAT"
// Current RFC 7235 auth-scheme used by aperture.
l402AuthScheme = "L402"
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One final nit: for a comment to be counted as a Godoc comment, it needs to start with the exact variable/field/method/function name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@guggero Thank you very much for the review! Great catch of file renames! I addressed the feedback. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One final nit, otherwise LGTM, nice work 💯
@@ -71,6 +71,11 @@ func (l *L402Authenticator) Accept(header *http.Header, serviceName string) bool | |||
return true | |||
} | |||
|
|||
const ( | |||
lsatAuthScheme = "LSAT" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One final nit: for a comment to be counted as a Godoc comment, it needs to start with the exact variable/field/method/function name.
Old clients expect "L402 macaroon=..." in the first WWW-Authenticate, while the protocol [1] says it should be "WWW-Authenticate: L402 macaroon=...", so send both LSAT and L402. LSAT must be sent first, to maintain backward compatibility with older clients. [1] https://github.com/lightninglabs/L402/blob/master/protocol-specification.md
Again, as with WWW-Authenticate header, existing aperture instances expect LSAT, and the protocol defines it is L402, so send both, LSAT first, to maintain backward compatibility. The header "Authorization: LSAT..." can be removed in the future, when all aperture instances are upgraded.
Create fresh http.Header object filled with the only header: "Content-Type: application/grpc".
The same regexp is then used in a call of FindStringSubmatch method and the outcome is the same, if nothing is found there.
This is needed to preserve old identifiers of nodes that have ever paid for a Loop or Pool token.
Thank you! Fixed the final nit :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, really nice work @starius 🥇
LSAT protocol has been renamed to L402.
This PR replaces all occurrences of "LSAT" with "L402" in the codebase.
Notes:
WWW-Authenticate: LSAT macaroon=
andAuthorization: LSAT ...
headers respectively. The protocol defines that they should both use the L402 auth-scheme instead of LSAT. Now, both of them send two headers: first, the LSAT version they used to send before, then the L402 version of the header. The latter contains the same content, but L402 is used as the auth-scheme instead of LSAT. The reason for preserving LSAT versions of the headers and sending them first is because existing instances of clients and servers check the first instance of a header. After Aperture is upgraded, we can stop sendingAuthorization: LSAT ...
, butWWW-Authenticate: LSAT ...
will remain at least until all currently deployed loop versions reach the end of life.Content-Type: application/grpc
.MatchString
call. It is followed byFindStringSubmatch
which does the same job and returnsnil
in case nothing matches. This results in the same outcome if the removedMatchString
call returns false.