-
Notifications
You must be signed in to change notification settings - Fork 11
Update liquidjs and code #145
base: master
Are you sure you want to change the base?
Update liquidjs and code #145
Conversation
src/transaction.ts
Outdated
@@ -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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this 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_
This reverts commit 5a20f54.
We still importing secp zkp, that should not be needed |
c76dcb6
to
81723f7
Compare
81723f7
to
94f0e62
Compare
Please review @tiero