fix(backend): allow admin api to query all receivers and move checks

[cozminu]

Changes proposed in this pull request

• moved validation checks (completed, expired and methods availability) to createOutgoingPayment, handleSending, resolveReceiver via Resolver.isInvalidExpiredOrCompleted

Context

fixes #3253

Checklist

• Related issues linked using fixes #number
• Tests added/updated
• Make sure that all checks pass
• Bruno collection updated (if necessary)
• Documentation issue created with user-docs label (if necessary)
• OpenAPI specs updated (if necessary)

[netlify]

✅ Deploy Preview for brilliant-pasca-3e80ec canceled.

Name	Link
🔨 Latest commit	33e5f8d
🔍 Latest deploy log	https://app.netlify.com/sites/brilliant-pasca-3e80ec/deploys/67c1b6c9e4e3db0008a6828c

[cozminu]

tests tbd after review

[BlairCurrey]

Does this need to be a static method? Or can we just make it a regular method, and use this to refer to the receiver instead of passing it in? This allows us to not import the Receiver model across our different services and leverages the receiver instance we already have.

Then instead of handling undefined in this method we check where this is called like:

So this:

      if (!receiver || receiver.isInvalidExpiredOrCompleted()) {
        throw OutgoingPaymentError.InvalidQuote
      }

Instead of

      if (Receiver.isInvalidExpiredOrCompleted(receiver)) {
        throw OutgoingPaymentError.InvalidQuote
      }

[cozminu]

It does not, I chose static for simplicity only. Changed to non-static method.

[BlairCurrey]

We can probably remove this payment method check. Its not going to do anything because the Receiver constructor will error if it's empty. It looks for ilp method then throws if it cant find it.

I am considering if it should be returned to the constructor but it seems kind of redundant in the first place. With or without a check for this, if method is empty, it will error as described here (missing ilp payment method). I guess the method.length check was just slightly more informative. I think just remove it here.

[cozminu]

moved to constructor for now because it makes more sense since there's already a check there and ILP method is hard-coupled to the Receiver via ilpAddress property.
Checked if this will affect querying the receiver via Admin API and it doesn't.

[BlairCurrey]

Can you speak more to this return type? I think maybe it should just be boolean instead? Perhaps it was working differently with the pattern of throwing errors but I don't think its really doing anything here. Nor could TS really know if its undefined or not.

This is what I see when using it (no effect on the receiver passed in).

const maybeReceiver = {} as Receiver | undefined

const r = Receiver.isInvalidExpiredOrCompleted(maybeReceiver)
r // type is boolean
maybeReceiver //type is still Receiver | undefined

[cozminu]

first time around I did it, function signature response was boolean

public static isActive(receiver: Receiver | undefined): boolean {

the issue was that I had to double check for type:

but if we do the non-static method that you suggested, then we won't have this problem because we will check for type before using the method

[cozminu]

when the signature response is receiver is Receiver and you have an if with a return or throw just like above, then typescript will know that the receiver below the if will be just of type Receiver (instead of Receiver | undefined)

[cozminu]

// function will return boolean, but typescript knows what to do with it
function isActive(r: Receiver | undefined): receiver is Receiver

const maybeReceiver = {} as Receiver | undefined

const r = Receiver.isActive(maybeReceiver)
r // type is boolean
maybeReceiver //type is still Receiver | undefined

if (!r) {
  return;
}

maybeReceiver //type is Receiver only

[BlairCurrey]

That makes sense, I see how its working now.

[njlie]

IncomingPayment.isExpiredOrComplete() isn't used anywhere else in the code with this deletion - perhaps it can be cleaned up?

[njlie]

nit: clean up comments