Skip to content
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

Merged
merged 3 commits into from
Apr 4, 2024

Conversation

mkurapov
Copy link
Contributor

@mkurapov mkurapov commented Apr 3, 2024

This reverts commit 81f8a05.

Changes proposed in this pull request

  • Reverting commit that introduced single phase transfers: we ended up having an issue of incoming payments not being able to complete, more details in Fix single phase transfer accounting #2629
  • Adding to integration test to make sure this case is tested

Context

Progress towards fixing #2629

Checklist

  • Related issues linked using fixes #number
  • Tests added/updated
  • Documentation added
  • Make sure that all checks pass
  • Bruno collection updated

@github-actions github-actions bot added type: tests Testing related pkg: backend Changes in the backend package. type: source Changes business logic labels Apr 3, 2024
Copy link

netlify bot commented Apr 3, 2024

Deploy Preview for brilliant-pasca-3e80ec canceled.

Name Link
🔨 Latest commit 661c0d4
🔍 Latest deploy log https://app.netlify.com/sites/brilliant-pasca-3e80ec/deploys/660e60e912b3dc0008b78d48

@sabineschaller sabineschaller changed the title Revert "feat(backend): support single phase transfer in connector (#2… chore: Revert "feat(backend): support single phase transfer in connector (#2… Apr 4, 2024
@mkurapov mkurapov changed the title chore: Revert "feat(backend): support single phase transfer in connector (#2… chore: Revert "feat(backend): support single phase transfer in connector Apr 4, 2024
@mkurapov mkurapov marked this pull request as ready for review April 4, 2024 08:16
@mkurapov mkurapov changed the title chore: Revert "feat(backend): support single phase transfer in connector chore: Revert "feat(backend): support single phase transfer in connector" Apr 4, 2024
expect(incomingPayment.receivedAmount).toMatchObject({
assetCode: 'EUR',
assetScale: 2,
value: 501n
Copy link
Contributor Author

@mkurapov mkurapov Apr 4, 2024

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?

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

@sabineschaller sabineschaller left a 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
Copy link
Member

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.

Copy link
Member

@sabineschaller sabineschaller left a 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... 🙃

@mkurapov mkurapov merged commit 1f763ed into main Apr 4, 2024
30 checks passed
@mkurapov mkurapov deleted the 2629/mk/revert-single-phase-change branch April 4, 2024 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: backend Changes in the backend package. type: source Changes business logic type: tests Testing related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants