More tests #12
More tests #12
Conversation
michielbdejong
force-pushed
the
mj-more-tests
branch
from
6502f45 to
f7523a5
Compare
src/index.js
Outdated
| return Promise.resolve() | ||
| const promises = [] | ||
| if (this.config.btp) { | ||
| promises.push(this.pluginFactory.stop()) |
There was a problem hiding this comment.
If there's only one promise, why use an array and Promise.all()?
There was a problem hiding this comment.
I agree it's currently superfluous, but I did it this way for symmetry with the start() function. Also in case we want to add a this.removePlugin function in the future.
| appliesToPrefix: curve.prefix, | ||
| sourceHoldDuration: 15000, | ||
| expiresAt: new Date(Date.now() + 3600 * 1000) | ||
| return Promise.resolve().then(() => { |
There was a problem hiding this comment.
What is the reason for this change?
There was a problem hiding this comment.
Because https://github.com/interledgerjs/amundsen/blob/master/src/request-handler.js#L34 requires this function to return a promise.
There was a problem hiding this comment.
That's not what I mean.
return Promise.resolve().then(() => {
return Promise.resolve(foo)
})
is redundant. You can just return Promise.resolve(foo) for the same effect.
| }) | ||
| } else { | ||
| this.main.getPlugin(prefix).rejectIncomingTransfer(transfer, { | ||
| code: 'L53', |
There was a problem hiding this comment.
Is there an appropriate error code from https://github.com/interledger/rfcs/blob/master/0003-interledger-protocol/0003-interledger-protocol.md#ilp-error-codes that could be used?
There was a problem hiding this comment.
Not really I think, which one were you thinking of?
There was a problem hiding this comment.
I didn't have one in mind, but the "L" prefix isn't used anywhere afaik. It should either be "F", "T", or "R" depending on what type of error it is. And probably 99 to indicate it is an application error.
There was a problem hiding this comment.
This code no longer exists.
|
Comments addressed and changes merged in #22 |
No description provided.