feat: raise error on no recipients eth musig rec and some refactors#51
feat: raise error on no recipients eth musig rec and some refactors#51pranavjain97 merged 2 commits intomasterfrom
Conversation
mtexeira-simtlix
force-pushed
the
WP-5185
branch
2 times, most recently
from
0342703 to
2ba8ee7
Compare
|
@pranavjain97 & @mohammadalfaiyazbitgo when reviewed merge the PR on this link first: as that contains a blocking issue that makes a testcase fail in master branch and then yes, proceed with this. |
mtexeira-simtlix
dismissed
mohammadalfaiyazbitgo’s stale review
The merge-base changed after approval.
mtexeira-simtlix
dismissed
mohammadalfaiyazbitgo’s stale review
The merge-base changed after approval.
| if (!recipients || recipients.length === 0) { | ||
| const errorMsg = `Recovery tx for coin ${coin} must have at least one recipient.`; | ||
| logger.error(errorMsg); | ||
| throw new Error(errorMsg); | ||
| } | ||
| } |
There was a problem hiding this comment.
If you're taking in the coin, is there a coin method that verifies the recipients? Either way - I don't see why you need a special helper for this. This can just be done directly in the main file.
There was a problem hiding this comment.
The helper was added because maybe in some other coin you need to check recipients between the unsigned -> half signed but also between half signed -> full.
Plus, a helper sometimes helps to communicate intention, the error msg polutes the intention with 3 lines of code and the helper just says what's the intent.
Thanks for the reviews!
| userPub, | ||
| backupPub, | ||
| apiKey, | ||
| unsignedSweepPrebuildTx: undefined, |
There was a problem hiding this comment.
bit confused on why this is undefined, and then in EBE you're checking if it contains recipients? 🤔
There was a problem hiding this comment.
That's not a refactor that i made, that comes from @mohammadalfaiyazbitgo previous refactor when he added the UTXO (got added here because i rebased his changes).
There was a problem hiding this comment.
But his PR is already merged; so the rebase shoudn't introduce any changes outside of your PR
| | 'localNonSecure' | ||
| | 'mock' | ||
| | 'adminProd' | ||
| | 'adminTest' | ||
| | 'adminStaging' | ||
| | 'adminDev' | ||
| | 'custom' | ||
| | 'branch'; |
There was a problem hiding this comment.
I thought we decided to cast? Adding all the other env's like admin.. here doesnt make sense since they dont apply to us.
There was a problem hiding this comment.
No, we decided to just extend it, i double checked that during the meet.
There was a problem hiding this comment.
extending why? we shoudn't have env names we dont have support for / dont apply in our case.
There was a problem hiding this comment.
As we talk in our quick sync (thanks for that!), i'm uploading the changes for casting instead of using the other envs.
pranavjain97
left a comment
pranavjain97
left a comment
There was a problem hiding this comment.
Did you test if ETH/UTXO recoveries are still working E2E with these changes?
3 participants
Ticket: WP-5185
Description: