Convert tokens #459
Convert tokens #459
Conversation
dangeross
changed the base branch from
main
to
savage-payment-update-persistence
dangeross
force-pushed
the
savage-token-transfers
branch
3 times, most recently
from
a67d4d5 to
eb80ff5
Compare
dangeross
force-pushed
the
savage-token-transfers
branch
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
crates/flashnet/src/auth.rs
Outdated
| let now = SystemTime::now().duration_since(UNIX_EPOCH).map_err(|_| { | ||
| FlashnetError::Generic("Failed to get current time".to_string()) | ||
| })?; | ||
| expires.saturating_sub(now).as_millis() |
There was a problem hiding this comment.
Should we add take several seconds as margin and not count on the exact time?
danielgranhao
left a comment
danielgranhao
left a comment
There was a problem hiding this comment.
Awesome! A few comments
| /// Conversion information if this was a successful conversion | ||
| conversion_info: Option<ConversionInfo>, | ||
| /// Conversion refund information if this was a conversion that was refunded | ||
| conversion_refund_info: Option<ConversionRefundInfo>, |
There was a problem hiding this comment.
Have you considered merging these 2 into an enum?
There was a problem hiding this comment.
It's split because of the two methods which they are synced. The conversion_info is synced from the Flashnet data and conversion_refund_info from rt-sync
There was a problem hiding this comment.
I think we could still merge into an enum even if in storage the variants are stored separately. Just an idea to avoid having to check 2 separate fields. I'm also ok with keeping like this.
| /// Only include payments matching these payment details filters | ||
| #[cfg_attr(feature = "uniffi", uniffi(default=None))] | ||
| pub spark_htlc_status_filter: Option<Vec<SparkHtlcStatus>>, | ||
| pub payment_details_filter: Option<PaymentDetailsFilter>, |
There was a problem hiding this comment.
Could we make it Option<Vec<PaymentDetailsFilter>> to allow both spark and token payments needing refund to be listed in a single call?
crates/breez-sdk/core/src/sdk.rs
Outdated
| } else { | ||
| error!( | ||
| "Failed to find payment for swap {} with inbound transfer id {}", | ||
| swap.id, swap.inbound_transfer_id | ||
| ); | ||
| } |
There was a problem hiding this comment.
IIUC, when we fail to find a payment for a swap, we ignore it (just log) and still mark it as synced. Isn't it a problem that we run sync_wallet and sync_conversion_info concurrently? Especially when recovering a wallet from scratch, it seems likely that we may try to look for a payment we haven't synced yet.
There was a problem hiding this comment.
I've removed sync_conversion_info from the parallel join, so it syncs after the wallet. I also now break out of the conversion info sync if the payment's not found and store the last updated offset. feaaf00
There was a problem hiding this comment.
Nice. I realize this is an edge case, but if, for any reason, a payment is missing and stays missing, we'll never fully complete syncing conversion info 🤔
Wondering how we may address that. Could we still store the conversion info in storage even if the corresponding payment is not there?
There was a problem hiding this comment.
We could put it in the storage cache. How likely do you think it is to happen?
Co-authored-by: Daniel Granhão <[email protected]>
This PR adds a means to convert funds to/from Bitcoin and another supported token.
TODO:
Closes #335