Aperture as a library - nice to have's #44
Conversation
ghost
mentioned this pull request
aperture.go
Outdated
| challenger, err = NewLndChallenger(cfg.Authenticator, | ||
| genInvoiceReq, | ||
| nil) |
There was a problem hiding this comment.
Nit: our preferred style here would be:
challenger, err = NewLndChallenger(
cfg.Authenticator, genInvoiceReq, nil,
)
challenger.go
Outdated
| // | ||
| // NOTE: This is part of the auth.InvoiceChecker interface. | ||
| func (l *LndChallenger) VerifyInvoiceStatus(hash lntypes.Hash, | ||
| state lnrpc.Invoice_InvoiceState, timeout time.Duration) error { | ||
| if l.verifyInvoiceStatus != nil { |
There was a problem hiding this comment.
Is this check necessary given that NewLndChallenger sets this to non-nil when provided a nil VerifyInvoiceStatus?
|
Thanks @wpaulino for the look. Added those changes, rebased, and cleaned up commits. |
| capabilities map[lsat.Service]lsat.Caveat | ||
| constraints map[lsat.Service][]lsat.Caveat | ||
| type StaticServiceLimiter struct { | ||
| Capabilities map[lsat.Service]lsat.Caveat |
There was a problem hiding this comment.
nit: Exported fields should have godoc comments.
| @@ -41,8 +41,8 @@ type InvoiceClient interface { | |||
| // LndChallenger is a challenger that uses an lnd backend to create new LSAT | |||
| // payment challenges. | |||
| type LndChallenger struct { | |||
| client InvoiceClient | |||
| genInvoiceReq InvoiceRequestGenerator | |||
| Client InvoiceClient | |||
There was a problem hiding this comment.
nit: exported fields should have godoc comments. This also removes the need for the large indentation as the fields will be separated a bit.
| @@ -21,6 +21,9 @@ import ( | |||
| // lnrpc.AddInvoice call. | |||
| type InvoiceRequestGenerator func(price int64) (*lnrpc.Invoice, error) | |||
|
|
|||
| type VerifyInvoiceStatusFunc func(hash lntypes.Hash, | |||
| @@ -84,15 +88,20 @@ func NewLndChallenger(cfg *authConfig, genInvoiceReq InvoiceRequestGenerator, | |||
| } | |||
|
|
|||
| invoicesMtx := &sync.Mutex{} | |||
| return &LndChallenger{ | |||
| c := &LndChallenger{ | |||
There was a problem hiding this comment.
Need to add the verifyInvoiceStatus to the struct, otherwise it won't be set if it's non-nil.
There was a problem hiding this comment.
Oh wow nice catch, I think while restructuring things I completely left it out here.
| Client: mockClient, | ||
| GenInvoiceReq: genInvoiceReq, | ||
| invoiceStates: make(map[lntypes.Hash]lnrpc.Invoice_InvoiceState), | ||
| quit: make(chan struct{}), | ||
| invoicesMtx: invoicesMtx, | ||
| invoicesCond: sync.NewCond(invoicesMtx), | ||
| errChan: mainErrChan, | ||
| }, mockClient, mainErrChan | ||
| } | ||
| c.VerifyInvoiceStatusFunc = c.DefaultVerifyInvoiceStatus |
There was a problem hiding this comment.
nit: can add directly to the struct initialization.
There was a problem hiding this comment.
I think there's a problem trying to assign it directly in the struct because it relies on an initialized challenge in DefaultVerifyInvoiceStatus. Wasn't sure how much of an anti pattern it was to do it the way I did or not but the side effect of it is that it can't be initialized directly.
|
Sorry about the delay on this, side project. I'll try to get the fixes in this weekend, thanks! |
|
@anthonyronning, remember to re-request review from reviewers for your latest update |
1 similar comment
|
@anthonyronning, remember to re-request review from reviewers for your latest update |
|
Closing due to inactivity. |
Discussion here: #45