Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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): add local payment #2857
feat(backend): add local payment #2857
Changes from 1 commit
bb77152
fc06e24
257fd01
d9fa20e
2e76ee2
4fab5d8
b17db75
6e37420
9ef7f94
320ee21
07569a7
f3d5f5d
d795ce2
251c60a
8f0bcb8
eca01f1
770d7ea
3849db7
170bcc3
88618ce
823c512
2e9f043
3fb02e5
569f029
dae1de1
2016469
3182944
3665188
5b8bad3
b49e08e
eedacb5
27a64b2
26d0539
2723961
08126c5
205447d
0471d84
7b5142d
773e4f2
0a3adf4
347f9b0
37d399e
78d42b6
a96d654
7941f05
753a57b
ed30066
8476615
f8a979c
5cee618
d41777e
e0f988d
5d631bd
ececad2
63011ae
bd12e30
b0eaf8d
0d69a3d
31e6cc7
bb9d334
b0897d9
2d8048a
15d8929
c40db9e
0dbb2d3
61b1010
0dea97d
6c995d9
86f6db6
3fc6924
6634b8f
6706557
998ee25
a4cf57a
1835bc4
7db833b
4437438
3149428
fe63991
e6cad1e
f120aab
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Thinking about this: since we are moving money between source <> destination asset in the end (e.g. USD > EUR) we actually need sourceAmount & sourceAsset to always be the sending currency (USD),
since in reality it may cost more for a certain provider to move USD > EUR vs EUR > USD.
Basically, if we need to get the
receiveAmount
, instead of using EUR <> USD rate, we would doconverted = ceil(receiveAmount / USD <> EUR rate)
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 hardcoded in the rates because I'm not sure where to get the sending currency (USD), since there is no
debitAmount
here. Also, I'm not actually sure how to trigger this block as opposed toreceiver.incomingAmount
further down. That's what the fixed send p2p example triggers. Although I think your comment applies to both places so that detail probably doesnt matter.Anyways, just wanted to validate this. With your calculation patched in and doing a fixed send local payment, I see an outgoing payment with these amounts:
The way it is now, the
receiveAmount
is the same anddebitAmount
andsentAmount
are 1 higher:For the same payment over ILP I see the same
1099
sentAmount
from your revised calculation but a differentdebitAmount
. Not sure if slippage would account for this (in which case there is no problem) or something else.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.
We should have this on the walletAddress.asset. So for those two branches without a debitAmount, we do the calculation as described above:
debitAmountValue = ceil(receiveAmount / (scaled USD <> EUR rate))
(scaled because we could have a difference in assetScale)
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.
Also, the estimated exchange rate can be the exchange rate that's received directly from the rates service, instead of having to do
estimatedExchangeRate: Number(receiveAmountValue) / Number(debitAmountValue)
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.
🤦 Of course, the
destinationAsset
passed in to the rates call.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.
convert function is same as before except this
reverseDirection
controls which conversion function is used (convert
or newconvertReverse
). Alternatively considered making a differentratesService.convertReverse
- perhaps thats a bit more explicit but I didn't have a strong preference and found this to be simpler.