Skip to content
This repository was archived by the owner on Jul 17, 2023. It is now read-only.

Update liquidjs and code#145

Open
Janaka-Steph wants to merge 11 commits intovulpemventures:masterfrom
Janaka-Steph:fix-confidential
Open

Update liquidjs and code#145
Janaka-Steph wants to merge 11 commits intovulpemventures:masterfrom
Janaka-Steph:fix-confidential

Conversation

@Janaka-Steph
Copy link
Copy Markdown

@Janaka-Steph Janaka-Steph commented Nov 14, 2022

Please review @tiero

}

export const DEFAULT_SATS_PER_BYTE = 0.1;
export const DEFAULT_SATS_PER_BYTE = 0.2;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise CI fails with "min relay fee not met, 124 < 130" https://github.com/vulpemventures/ldk/actions/runs/3461702660/jobs/5779643032

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I changed this a bit too fast, I thought it would affect just the tests. Maybe this is better https://github.com/vulpemventures/ldk/pull/143/files#diff-daff4cb5ab57a2886a0cfe4dae670b9f75c84e865e6dd147a5ad8976ea51924cR194 ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this tells me our tx estimation may not be right. Which type of script it's failing?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Failing the transaction test, in craftSingleRecipientPset() I think

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok best to tweak from code, rather than change the constant. Will investigate eventually why estimation is changing

Copy link
Copy Markdown
Member

@tiero tiero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since now also ZKP is injectable like normal secp, I would do it expecting a ZKPInterface as we do for TinySecp256k1Interface in options when instantiating the Identity with ecclib.

There is a pending discussion to merge methods of TinySecp in ZKPInterface this way we could pass only secp256k1-zkp to LDK and other libraries using LiquidJS in general, since ZKP would contain all methods ecclib needs.
vulpemventures/liquidjs-lib#85

I think for now we can add a temporary zkplib field in the Identity interface?

ie. @vv/secp256k1-zkp would then only be used in tests, and not imported when installing the LDK library. This would benefit environments without wasm such as react native (they will bring their own zkplib_

@Janaka-Steph Janaka-Steph requested a review from tiero November 14, 2022 14:59
@Janaka-Steph Janaka-Steph requested a review from tiero November 14, 2022 18:14
@tiero
Copy link
Copy Markdown
Member

tiero commented Nov 14, 2022

We still importing secp zkp, that should not be needed

@Janaka-Steph Janaka-Steph requested a review from tiero November 14, 2022 18:24
@Janaka-Steph Janaka-Steph force-pushed the fix-confidential branch 2 times, most recently from c76dcb6 to 81723f7 Compare November 14, 2022 21:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants