-
Notifications
You must be signed in to change notification settings - Fork 89
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
feat(backend): track if receiver is local #2862
Conversation
✅ Deploy Preview for brilliant-pasca-3e80ec ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
try { | ||
const localIncomingPayment = await getLocalIncomingPayment({ | ||
deps, | ||
url | ||
}) | ||
if (localIncomingPayment) { | ||
return new Receiver(localIncomingPayment, true) | ||
} | ||
|
||
const remoteIncomingPayment = await getRemoteIncomingPayment(deps, url) | ||
if (remoteIncomingPayment) { | ||
return new Receiver(remoteIncomingPayment, false) | ||
} | ||
} catch (error) { | ||
deps.logger.error( | ||
{ errorMessage: error instanceof Error && error.message }, | ||
'Could not get incoming payment' | ||
) | ||
} | ||
|
||
return undefined |
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.
The real change here in getReceiver
is that we create the receiver with isLocal
. The other changes in the receiverService.get
are a little refactor in support of this. Just moves the attempt to get the local incoming payment into this main getReceiver
function so I dont have to return more stuff from getIncomingPayment
(now getRemoteIncomingPayment
, since thats all it does).
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.
Generally looks good. Makes code cleaner. Just a question regarding the tests. You updated them by always setting isLocal = false
. Should we also test a true
case?
@sabineschaller I changed the receiver model test to true for the sake of this (although it probably doesnt matter much - we're already asserting that its false when we set to false in constructor). There's really just not much to test here since we're only adding a new property and not controlling anything with it. I dont want to change any of the other tests since everything up until this point was implicitly non-local. I think designating as local might not make sense there and could cause them to fail unexpectedly when we make |
Changes proposed in this pull request
isLocal
boolean to Receiver constructorContext
This will be utilized to determine if we should use the local or ilp payment methods. I made this a separate PR since it's not dependent on any of the other changes and should help keep the other PR for that task (a bit) more manageable.
fixes #2853
Checklist
fixes #number