[r2r] lightning ordermatching wip + library updates and more unit tests by shamardy · Pull Request #1655 · GLEECBTC/komodo-defi-framework

shamardy · 2023-02-13T20:43:10Z

Intermediate PR as part of #1045

Updated rust-lightning to v0.0.113
Added channel current_confirmations and required_confirmations (which are easy to retrieve after the above update) to channel details RPCs.
Used uuid for internal channel id instead of u64 after rust-lightning update since they now use u128 for user_channel_id
Updated uuid lib to latest version to use conversion from/to u128 features
Added unit tests for multi path payments.
Added unit tests for claiming swaps on-chain for closed channels which is not currently working. After some discussion with rust-lightning devs this will be implemented in rust-lightning instead of AtomicDEX Support claiming from closed channels lightningdevkit/rust-lightning#2017
Started working in lightning ordermatching, protocol_info fields are used to check if an order can be matched or not depending on if a route is found between maker/taker for a lightning payment.
Fixed 2 issues I discovered while executing a KMD/LNBTC swap, they were:
- When electrums were down, if a channel was closed, the channel closing transaction wasn't broadcasted. Now I check for a network error and rebroadcast transactions after a delay if there was a network error.
- If an invoice payment failed, if I retry paying the same invoice later, the payment wasn't updated to successful in DB if it were. Now I added a method to update the payment in the DB to update it in this case.

To be done in next PRs:

Complete lightning order matching implementation and balance checks.
- Right now inbound liquidity is not checked when placing an order to receive lightning BTC.
- Show lightning node address in orderbook/best_orders response.
- Fees can't be calculated accurately before an order is placed since they depend on the route. Will need to find a workaround for this.
After updating to v0.0.113, an async event handler can be used instead of background processor, this paves the way for lightning to work in WASM.

…e chain, test is failing for now

…ng channels

…e not routes between swap parties

…check in lightning protocol info check

…bled and channel is closed before funding tx is confirmed

onur-ozkan

Amazing work! My notes from the partial review

onur-ozkan · 2023-02-16T10:45:06Z

+    let expect_msg = "for the foreseeable future this shouldn't happen";
+    let current_time: u32 = get_local_duration_since_epoch()
+        .expect(expect_msg)
+        .as_secs()
+        .try_into()
+        .expect(expect_msg);


Suggested change

let expect_msg = "for the foreseeable future this shouldn't happen";

let current_time: u32 = get_local_duration_since_epoch()

.expect(expect_msg)

.as_secs()

.try_into()

.expect(expect_msg);

let current_time: u32 = get_local_duration_since_epoch()

.map_to_mm(|e| EnableLightningError::Internal(e.to_string()))?

.as_secs()

.try_into()

.map_to_mm(|e| EnableLightningError::Internal(format!("{:?}", e)))?;

onur-ozkan · 2023-02-16T10:58:33Z

-    /// Get some of the coin config info in serialized format for p2p messaging.
-    fn coin_protocol_info(&self) -> Vec<u8>;
+    /// Get some of the coin protocol related info in serialized format for p2p messaging.
+    fn coin_protocol_info(&self, amount_to_receive: Option<MmNumber>) -> Vec<u8>;


Some of the implementations returning empty byte array, and this value is usually passed as Option type to the target. I am not sure if Some(Vec::new()) is handled properly. I would rather update the return type into Option<Vec<u8>> here and return None for functions that are returning Vec::new().

coin_protocol_info first implementation used to return Option<Vec<u8>> and was changed in the following PR #1064 by Artem. We had some backward compatibility issues related to this before #1017, #1046 that would make me hesitant to change this.

@shamardy I think in most cases, an empty vec == None...what I'm saying it's unnecessary to use Option<Vec<_>> but instead just Vec<_>.

seems this is how optional keys field are mostly stored in structures mm2 so no problem ):

onur-ozkan · 2023-02-16T11:30:01Z

+[[package]]
+name = "bech32"
+version = "0.9.1"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "d86b93f97252c47b41663388e6d155714a9d0c398b99f1005cbc5f978b29f445"


Only dependency that is still using 0.8.1 was KomodoPlatform/librustzcash, I upgraded bech32 to 0.9.1 there in k-1.0.0 tag. So we can avoid having 2 versions of bech32 that are almost identical.

You can apply the following patch for the version migration:

diff --git a/mm2src/coins/Cargo.toml b/mm2src/coins/Cargo.toml index f35490716..1aa4824dc 100644 --- a/mm2src/coins/Cargo.toml +++ b/mm2src/coins/Cargo.toml @@ -125,10 +125,10 @@ tokio = { version = "1.7" } tokio-rustls = { version = "0.23" } tonic = { version = "0.7", features = ["tls", "tls-webpki-roots", "compression"] } webpki-roots = { version = "0.22" } -zcash_client_backend = { git = "https://github.com/KomodoPlatform/librustzcash.git" } -zcash_client_sqlite = { git = "https://github.com/KomodoPlatform/librustzcash.git" } -zcash_primitives = { features = ["transparent-inputs"], git = "https://github.com/KomodoPlatform/librustzcash.git" } -zcash_proofs = { git = "https://github.com/KomodoPlatform/librustzcash.git" } +zcash_client_backend = { git = "https://github.com/KomodoPlatform/librustzcash.git", tag = "k-1.0.0" } +zcash_client_sqlite = { git = "https://github.com/KomodoPlatform/librustzcash.git", tag = "k-1.0.0" } +zcash_primitives = { features = ["transparent-inputs"], git = "https://github.com/KomodoPlatform/librustzcash.git", tag = "k-1.0.0" } +zcash_proofs = { git = "https://github.com/KomodoPlatform/librustzcash.git", tag = "k-1.0.0" } [target.'cfg(windows)'.dependencies] winapi = "0.3"

Thanks a lot for upgrading librustzcash. Patch applied.

…for current_time

caglaryucekaya

Great work! Some questions and comments

onur-ozkan

lgtm! just one suggestion

…ed due to a network error (all electrums were down, etc..)

shamardy · 2023-03-03T17:39:38Z

I fixed 2 issues I discovered while executing a KMD/LNBTC swap, they were:

When electrums were down, if a channel was closed, the channel closing transaction wasn't broadcasted. Now I check for a network error and rebroadcast transactions after a delay if there was a network error.
If an invoice payment failed, if I retry paying the same invoice later, the payment wasn't updated to successful in DB if it were. Now I added a method to update the payment in the DB to update it in this case.

@caglaryucekaya @ozkanonur can you please review the new commits?

onur-ozkan

@ozkanonur can you please review the new commits?

Reviewed f7953a7 and e683bea

Both looks good to me

shamardy · 2023-03-07T15:09:31Z

@caglaryucekaya can you please approve if there are no more issues with this PR?

caglaryucekaya

LGTM! Just two comments

caglaryucekaya · 2023-03-07T15:43:31Z

+                        if !e.get_inner().is_network_error() {
+                            break;
+                        }
+                        Timer::sleep(TRY_LOOP_INTERVAL).await;


Should this loop repeat infinitely if the network error keeps coming?

Yes for now. When broadcasting transaction through p2p network is implemented, a loop to check if the transaction is on-chain or not can be added instead.

caglaryucekaya · 2023-03-07T15:49:25Z

+                        error!("Converting transaction to bytes error:{}", e);
+                        break;
+                    },
+                };


I think you can take this out of the loop, there's no need to repeat it in case of network failure

…dcasterInterface

caglaryucekaya

Great work 🔥

borngraced · 2023-03-08T07:32:54Z

 pub enum SelectRecentSwapsUuidsErr {
    Sql(SqlError),
-    Parse(uuid::parser::ParseError),
+    Parse(uuid::Error),


uuid::Error is used in multiple places already so it's better to import once

use uuid::Error as UuidError;

and use everywhere

borngraced · 2023-03-08T07:33:11Z


-impl From<uuid::parser::ParseError> for SelectRecentSwapsUuidsErr {
-    fn from(err: uuid::parser::ParseError) -> Self { SelectRecentSwapsUuidsErr::Parse(err) }
+impl From<uuid::Error> for SelectRecentSwapsUuidsErr {


UuidError

borngraced

Great work! just a note and suggestion.

borngraced

Great job and welldone!

* add change logs for PRs merged to dev only * remove {version} - {date} and add dev branch instead * add ibc transfer change logs * add lightning PR #1655 to change logs * add changelog for #1706 * add changelog for #1694, #1665 --------- Reviewed-by: laruh, caglaryucekaya <caglaryucekaya@gmail.com>

shamardy added 15 commits January 19, 2023 14:07

add a unit test to test lightning taker getting swap preimage from th…

3965444

…e chain, test is failing for now

update rust-lightning to v0.0.111 wip

5c94a9f

update rust-lightning to v0.0.113 wip

12f265c

update uuid to v1.2.2, use uuid instead of rpc_channel_id for lightni…

fe0189e

…ng channels

add unit tests for mpp and claiming swaps on-chain

0a9e017

Add channel confirmation details to rpc response

31737cf

add test_lightning_maker_swap_mpp

65655c7

wip: use protocol info to not match with lightning orders if there ar…

29536be

…e not routes between swap parties

Merge remote-tracking branch 'origin/dev' into ln-onchain-swaps

f07a9d1

wip: add amount to protocol info route check

dfa04f0

wip: refactor lightning protocol info code

e64f422

Add max_total_cltv_expiry_delta and final_cltv_expiry_delta to route …

8fdba01

…check in lightning protocol info check

Fix funding_generated_in_block is Null in DB error when 0 conf is ena…

155ff64

…bled and channel is closed before funding tx is confirmed

remove some unneeded todos, write better todos

d7a0c34

remove more todos

268f2c6

shamardy changed the title ~~lightning ordermatching wip + library updates and more unit tests~~ [r2r] lightning ordermatching wip + library updates and more unit tests Feb 13, 2023

shamardy requested review from ca333, caglaryucekaya and onur-ozkan February 13, 2023 20:55

onur-ozkan suggested changes Feb 16, 2023

View reviewed changes

shamardy changed the title ~~[r2r] lightning ordermatching wip + library updates and more unit tests~~ [wip] lightning ordermatching wip + library updates and more unit tests Feb 17, 2023

review fixes: use bech32 0.9.1, return error instead of using expect …

a88e3ea

…for current_time

shamardy changed the title ~~[wip] lightning ordermatching wip + library updates and more unit tests~~ [r2r] lightning ordermatching wip + library updates and more unit tests Feb 17, 2023

caglaryucekaya reviewed Feb 17, 2023

View reviewed changes

Comment thread mm2src/coins/lightning.rs Outdated

Comment thread mm2src/coins/lightning/ln_serialization.rs

Comment thread mm2src/coins/lightning.rs

Comment thread mm2src/coins/lightning.rs

onur-ozkan previously approved these changes Feb 21, 2023

View reviewed changes

Comment thread mm2src/coins/lightning.rs

review fixes: use macro to simplify code, sort by short_channel_id

9f3803e

shamardy dismissed onur-ozkan’s stale review via 9f3803e February 21, 2023 22:56

shamardy added 3 commits February 22, 2023 01:06

review fixes: add doc comments for uuid and channel_id

d37b6cc

Merge remote-tracking branch 'origin/dev' into ln-onchain-swaps

9577efc

Fix channel was closed issue but closing transaction wasn't broadcast…

f7953a7

…ed due to a network error (all electrums were down, etc..)

shamardy changed the title ~~[r2r] lightning ordermatching wip + library updates and more unit tests~~ [wip] lightning ordermatching wip + library updates and more unit tests Mar 3, 2023

fix issue in db when retrying to pay an invoice

e683bea

shamardy changed the title ~~[wip] lightning ordermatching wip + library updates and more unit tests~~ [r2r] lightning ordermatching wip + library updates and more unit tests Mar 3, 2023

onur-ozkan previously approved these changes Mar 6, 2023

View reviewed changes

Merge remote-tracking branch 'origin/dev' into ln-onchain-swaps

da8d3ad

caglaryucekaya reviewed Mar 7, 2023

View reviewed changes

move converting tx hex to bytes outside send transaction loop in Broa…

f92d69a

…dcasterInterface

shamardy dismissed onur-ozkan’s stale review via f92d69a March 7, 2023 16:21

Merge remote-tracking branch 'origin/dev' into ln-onchain-swaps

c5dd404

caglaryucekaya previously approved these changes Mar 7, 2023

View reviewed changes

borngraced reviewed Mar 8, 2023

View reviewed changes

borngraced suggested changes Mar 8, 2023

View reviewed changes

import uuid::Error as UuidError in my_swaps.rs

0daed47

shamardy dismissed caglaryucekaya’s stale review via 0daed47 March 9, 2023 14:45

borngraced approved these changes Mar 9, 2023

View reviewed changes

shamardy merged commit ebdc8c2 into dev Mar 10, 2023

shamardy deleted the ln-onchain-swaps branch March 10, 2023 15:30

shamardy added a commit that referenced this pull request Mar 10, 2023

add lightning PR #1655 to change logs

e97444c

ca333 mentioned this pull request Mar 27, 2023

[release] 1.0.1-beta #1721

Merged

Conversation

shamardy commented Feb 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

onur-ozkan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shamardy Feb 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caglaryucekaya left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

onur-ozkan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shamardy commented Mar 3, 2023

Uh oh!

onur-ozkan left a comment

Choose a reason for hiding this comment

Uh oh!

shamardy commented Mar 7, 2023

Uh oh!

caglaryucekaya left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caglaryucekaya left a comment

Choose a reason for hiding this comment

Uh oh!

borngraced Mar 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

borngraced left a comment

Choose a reason for hiding this comment

Uh oh!

borngraced left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

shamardy commented Feb 13, 2023 •

edited

Loading

shamardy Feb 17, 2023 •

edited

Loading

borngraced Mar 8, 2023 •

edited

Loading