Prep for token to lightning payments #459
Conversation
dangeross
changed the base branch from
main
to
savage-payment-update-persistence
dangeross
force-pushed
the
savage-token-transfers
branch
4 times, most recently
from
eb80ff5 to
54047cb
Compare
| "Amount in {amount_in} is less than minimum required {min_in} for asset {asset_in_address}", | ||
| ))); | ||
| } | ||
| return Ok(()); |
There was a problem hiding this comment.
We skip here without checking the min_amount_out ?
There was a problem hiding this comment.
Copied logic from the Flashnet SDK. You think it's worth checking?
There was a problem hiding this comment.
To me it looks like the min_amount_out is not validated in this case. It looks like a bug unless it is not used when min_in is set.
There was a problem hiding this comment.
Yeah, I was assuming if there's a min in to check and it's valid, we don't need to check the min out also
danielgranhao
left a comment
danielgranhao
left a comment
There was a problem hiding this comment.
Awesome! A few comments
dangeross
force-pushed
the
savage-token-transfers
branch
2 times, most recently
from
bffa205 to
c728a88
Compare
crates/breez-sdk/core/src/sdk.rs
Outdated
|
|
||
| tokio::join!(sync_wallet, sync_lnurl, sync_deposits); | ||
|
|
||
| if sync_type.contains(SyncType::ConversionInfo) { |
There was a problem hiding this comment.
My concern here is that we always attempting to approach flashnet API while lots of users won't use it. Slowing the sync for most cases for no reason.
Perhaps we can add a config flag (enable_flashnet) to signal we need to sync with it?
roeierez
left a comment
roeierez
left a comment
There was a problem hiding this comment.
Looks a lot better. I still plan to do another review.
crates/breez-sdk/core/src/sdk.rs
Outdated
| let pools_response = self | ||
| .flashnet_client | ||
| .list_pools(ListPoolsRequest { | ||
| asset_a_address: Some(asset_in_address.clone()), | ||
| asset_b_address: Some(asset_out_address.clone()), | ||
| sort: Some(PoolSortOrder::Volume24hDesc), | ||
| ..Default::default() | ||
| }) | ||
| .await?; | ||
| // Get the pool for the conversion | ||
| let pool = pools_response.pools.first().ok_or(SdkError::Generic( | ||
| "No pool found for the given token identifier".to_string(), | ||
| ))?; | ||
| let response = self | ||
| .flashnet_client | ||
| .simulate_swap(SimulateSwapRequest { | ||
| asset_in_address: asset_in_address.clone(), | ||
| asset_out_address: asset_out_address.clone(), | ||
| pool_id: pool.lp_public_key, | ||
| amount_in: request.amount, | ||
| integrator_bps: None, | ||
| }) | ||
| .await?; |
There was a problem hiding this comment.
Now that we don't need to sync we can treat flashnet as a swapper (swap provider) from the sdk perspective and this code (and some more )could be extracted out to its own swapper trait/service. It it a nice to have for now that will get the flashnet specifics out of the sdk generic code. I wouldn't hold the merge for this feedback.
There was a problem hiding this comment.
Maybe it could even be a plugin? Then, even multiple swappers could be used by the same SDK instance
There was a problem hiding this comment.
I agree but I would rather wait for another implementation before attempting to generalize as a plugin. This has complexities also around integration with the payment list, sync etc... perhaps it is too soon.
dangeross
force-pushed
the
savage-payment-update-persistence
branch
from
0823ef4 to
08dd127
Compare
dangeross
force-pushed
the
savage-token-transfers
branch
from
7e63851 to
3316594
Compare
Preparation PR for token to lightning payments