Address Issue #171: Validate Generated Addresses Against Expected Ones in JS Client Library #184
Conversation
…ed using npm install
…nternally) without interfering with the original package.json
There was a problem hiding this comment.
Thanks for the PR, great to be able to validate addresses at least on SegWit!
Few comments to align with the other libraries:
- The approach I used in the other libraries is to check the address both at the end of get_wallet_address (no performance impact), but also at the end of register_wallet. This could have a slight performance impact, as generating an address is explicit extra work. however, this is probably negligible in practice.
validateAddresscan completely replace the existingvalidatePolicy, since both the bugs that are checked are for SegWit miniscript. ThecontainsAhelper can also be deleted.
Feel free to bump the package version to 0.2.3, there's no plan to keep the version aligned across clients.
I just realised that adding the check mentioned above will break the test for it("can register a miniscript wallet", async () => {
const walletPolicy = new WalletPolicy(
"Decaying 3-of-3",
"wsh(thresh(3,pk(@0/**),s:pk(@1/**),s:pk(@2/**),sln:older(12960)))",
[
"[76223a6e/48'/1'/0'/2']tpubDE7NQymr4AFtewpAsWtnreyq9ghkzQBXpCZjWLFVRAvnbf7vya2eMTvT2fPapNqL8SuVvLQdbUbMfWLVDCZKnsEBqp6UK93QEzL8Ck23AwF",
"[f5acc2fd/48'/1'/0'/2']tpubDFAqEGNyad35aBCKUAXbQGDjdVhNueno5ZZVEn3sQbW5ci457gLR7HyTmHBg93oourBssgUxuWz1jX5uhc1qaqFo9VsybY1J5FuedLfm4dK",
]
);
const automation = JSON.parse(fs.readFileSync('src/__tests__/automations/register_wallet_accept.json').toString());
await setSpeculosAutomation(transport, automation);
const [walletId, walletHmac] = await app.registerWallet(walletPolicy);
expect(walletId).toEqual(walletPolicy.getId());
expect(walletHmac.length).toEqual(32);
}, 60000);It's worth mentioning that the template requires 3 keys, but only 2 keys are provided. Generating an address with an incomplete set of keys isn't feasible. Is this a legitimate policy anyway? If so, how should we approach this circumstance? EDIT: I decided to add an additional key and have submitted a new commit reflecting this. Please see my most recent message below for an overview of the changes. |
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## develop #184 +/- ##
===========================================
- Coverage 84.50% 84.27% -0.24%
===========================================
Files 17 17
Lines 2162 2168 +6
===========================================
Hits 1827 1827
- Misses 335 341 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Hi @bigspider , I've made the changes you suggested. Specifically, I've:
Please let me know if you have any additional feedback. |
| // the `a:` fragment. | ||
| throw new Error("Please update your Ledger Bitcoin app.") | ||
| } | ||
| /* Performs any additional check on the genetated address before returning it.*/ |
There was a problem hiding this comment.
Typo in comment: genetated.
There was a problem hiding this comment.
Oups, it looks like the rebase+commit did not go as expected. Hang on while I figure out how to solve this.
For some reasone there appear a list of old commits I've made in the past. I believe this is wrong. I think it may be better to start a new PR to sort this out (unless this is ok or you know how I can fix it).
… end of registerWallet; validateAddress replaces validatePolicy; bumped package version; correctly deal with <M;N> format (added a test using this format); Fixed incomplete keys in registerWallet test
Revised the key replacement logic in the `validateAddress` function to prevent misinterpretation of key indices. The loop now iterates in reverse order to avoid scenarios where, for example, @10 is mistakenly replaced as @1, leaving an extra 0. This change ensures that the correct keys are replaced when deriving the wallet address.
landabaso
force-pushed
the
improved-validation
branch
from
5209994
to
8cf4162
Compare
Replace PR #184: Validate Generated Addresses Against Expected Ones in JS Client Library
This PR aims to address issue #171 by implementing an enhanced validation method in the JavaScript client library. It enables the comparison of the generated wallet addresses with the expected ones, leveraging "@bitcoinerlab/descriptors". Here are the key updates:
Updated package.json to incorporate new dependencies "@bitcoinerlab/descriptors" and "@bitcoinerlab/secp256k1", and the latest version of "bitcoinjs-lib" to "^6.1.3".
Introduced a new test in appClient.test.ts to validate the generated address or throw an error if it doesn't match the expected one.
Made modifications in appClient.ts to integrate new imports. Changes have been made to the
AppClientclass to use these libraries for address validation. Some previous validations were removed (see below for further discussion).Note that this PR utilizes "@bitcoinerlab/descriptors" for validation purposes. It offers an effective approximation for validating addresses. You can view the extensive set of tests that bitcoinerlab passes here.
However, it's worth noting that "@bitcoinerlab/descriptors" does not yet support Tapscript yet. As a result, this third-party validation may not be available for certain types of descriptors. The code takes this into account and handles these cases appropriately.
In
getWalletAddress, the previous validation method has been replaced with the new approach comparing generated addresses to expected ones using "@bitcoinerlab/descriptors".Meanwhile, I'd like to discuss the removal of validation in the
registerWalletandsignPSBTmethods of theAppClientclass. An alternative approach could be to generate addresses for dummy keys and compare them using thevalidateAddressmethod. However, this might be excessive, potentially slow down performance, and there might be other drawbacks I'm not considering. I look forward to your comments on this proposal.Regarding the versioning in package.json, I've decided not to update the version number. It appears that the version number of the npm package has been synchronized with the version of the entire repository which includes Rust and Python clients as well. This leads to a situation where a fix in TypeScript would have to wait for a new release version of the whole project before it could be submitted to npm.
I look forward to your feedback.
Closes #171.