-
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
chore: Revert "feat(backend): support single phase transfer in connector" #2630
Conversation
)" This reverts commit 81f8a05.
✅ Deploy Preview for brilliant-pasca-3e80ec canceled.
|
expect(incomingPayment.receivedAmount).toMatchObject({ | ||
assetCode: 'EUR', | ||
assetScale: 2, | ||
value: 501n |
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.
I understand that the receivedAmount may be greater than the sentAmount during cross currency transactions (i.e. you need at least 0.02 USD to reach a minimum of 0.01 EUR, if USD < EUR).
However, this receivedAmount feels wrong to me:
original receiveAmount = 500
sentAmount = 550
receivedAmount = 501
with an exchange rate of 0.91 as tested above, we have 550 * 0.91 = 500.5, which is under the final receivedAmount of 501. This signifies somewhere in ILP pay there is a Math.ceil
that ends up making money out of nothing. Are my assumptions correct in the fact that this seems like a problem?
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.
I was about to say that this could be because the stream was closed too late but given that the packet size is huge, that can't be it. I guess we have a rounding problem somewhere.
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.
Unfourtunately will need to dive in deeper into ILP pay, since this is just the default behaviour. Will merge in for now, since it is the status quo at this point anyway.
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.
Thanks for adding the additional tests!
expect(incomingPayment.receivedAmount).toMatchObject({ | ||
assetCode: 'EUR', | ||
assetScale: 2, | ||
value: 501n |
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.
I was about to say that this could be because the stream was closed too late but given that the packet size is huge, that can't be it. I guess we have a rounding problem somewhere.
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.
I don't know why I didn't approve earlier... 🙃
This reverts commit 81f8a05.
Changes proposed in this pull request
Context
Progress towards fixing #2629
Checklist
fixes #number