feat(jellyfish-transaction-builder): Extend createDeFiTx to allow for custom vins and vouts #1892
Conversation
marktanrj
requested review from
fuxingloh,
ivan-zynesis and
canonbrother
as code owners
|
Code Climate has analyzed commit ae7b3e5 and detected 1 issue on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
✅ Deploy Preview for jellyfishsdk ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
Codecov ReportBase: 89.95% // Head: 91.24% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1892 +/- ##
==========================================
+ Coverage 89.95% 91.24% +1.29%
==========================================
Files 358 359 +1
Lines 10432 10464 +32
Branches 1317 1321 +4
==========================================
+ Hits 9384 9548 +164
+ Misses 1012 879 -133
- Partials 36 37 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
Docker build preview for jellyfish/apps is ready! Built with commit ec9a4f5
You can also get an immutable image with the commit hash
|
| */ | ||
| async createDeFiTx ( | ||
| opDeFiTx: OP_DEFI_TX, | ||
| changeScript: Script, | ||
| outValue: BigNumber = new BigNumber('0') | ||
| outValue: BigNumber = new BigNumber('0'), | ||
| customVinVout: Array<{ vin: Vin, vout: Vout, prevout: Prevout }> = [] |
There was a problem hiding this comment.
This overloader concept isn't really applicable and it doesn't gel well.
- Given that can inject any
Vin, theVincan already be collected byawait this.collectPrevouts(minFee)and included in vins, giving you an invalid Transaction due to duplicated Vins. - The assumption is with the
Vinprovided that you have the signing rights to it. - That
{Vin, Vout, Prevout}comes in a Set, that's not true since onlyVinis tied toPrevout. The amountVintoVoutratio is not exclusive. So an array won't make sense.
There was a problem hiding this comment.
We also have to ensure collectPrevout doesn't collect the Vin you need to sign in a special way.
There was a problem hiding this comment.
Your setup requires:
- A custom "Prevouts Collector" implementation since you need control of the
Vins - Injection of
Voutin a special order.
There was a problem hiding this comment.
Would it make sense for the upstream implementation to just prepare their Transaction from scratch?
We change protected async sign to public sync sign to allow that. And anything else that is required we make it public or create an abstraction for it.
There was a problem hiding this comment.
Hmm, a little lost 😓 correct me if I'm wrong:
- I will implement a "Prevouts Collector" as a method in
P2WPKHTxnBuilderclass - then combine the outputs of it along with outputs of
await this.collectPrevouts(minFee)so Vin and Vout can be placed in the correct order that doesn't have duplicates?
There was a problem hiding this comment.
prepare their Transaction from scratch
do you mean build the transaction from scratch with vin and vout and then sign it, all in the updatemasternode function in packages/jellyfish-transaction-builder/src/txn/txn_builder_masternode.ts?
There was a problem hiding this comment.
Yup, that's right. Since your requirements require vin and vout to be arranged in a certain way. Nobody else will probably need it. It is better to keep the custom logic there instead of bleeding it into an abstraction.
There was a problem hiding this comment.
Ok, that makes sense. I will do it this way since no other functions need it 👍
|
I will build the updatemasternode transaction from scratch in #1894 in |
… builder (#1894) <!-- Thanks for sending a pull request! --> #### What this PR does / why we need it: - updatemasternode transaction builder #### Which issue(s) does this PR fixes?: <!-- (Optional) Automatically closes linked issue when PR is merged. Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`. --> Fixes part of #1842 #### Additional comments?: - #1863 needed for typings - <del> #1892 needed for createDeFiTx </del> Co-authored-by: Isaac Yong <[email protected]>
What this PR does / why we need it: