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

Update liquidjs and code #145

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

Janaka-Steph
Copy link

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

Please review @tiero

@@ -90,7 +90,7 @@ export interface BuildTxArgs {
errorHandler?: CoinSelectorErrorFn;
}

export const DEFAULT_SATS_PER_BYTE = 0.1;
export const DEFAULT_SATS_PER_BYTE = 0.2;
Copy link
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
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
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
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
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
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
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_

src/identity/identity.ts Outdated Show resolved Hide resolved
@tiero
Copy link
Member

tiero commented Nov 14, 2022

We still importing secp zkp, that should not be needed

src/identity/identity.ts Outdated Show resolved Hide resolved
src/identity/identity.ts Outdated Show resolved Hide resolved
@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