Add unit test for method ExternalKey.PubKey #1284

ffranr · 2025-01-09T12:21:19Z

This PR introduces a comprehensive unit test for the PubKey method of the ExternalKey struct. The test covers various scenarios, including valid and invalid derivation paths, ensuring the method's correctness and error handling.

Notes for Reviewers

This PR depends on #1272 . This PR should be merged after #1272 has been merged. I will keep in draft until #1272 is merged.

ffranr · 2025-01-09T12:22:51Z

Need a BIP86 xpub and master fingerprint. I don't think it's possible with https://iancoleman.io/bip39/ ?

ffranr · 2025-01-09T14:03:52Z

We can get a real BIP86 xpub set from the chantools itest:

  chantools_harness.go:108: Chantools derivekey output: 2025-01-09 14:02:16.974 [INF] CHAN: chantools version v0.13.5 commit 
        
        Path:				m/86'/1'/0'
        Network: 			regtest
        Master Fingerprint:             279eeee3
        Public key: 			03b33a2a53dcf579282b3dd2d20981b6a707f8e1ec4983e5896dba5f4283277d39
        Extended public key (xpub): 	tpubDDfTBtwwqxXuCej7pKYfbXeCW3inAtv1cw4knmvYTTHkw3NoKaeCNH5XdY6n6fnBPc1gWEgeurfmBVzJLfBB1hGU64LsHFzJv4ASqaHyALH
        Address: 			bcrt1qmlsx2ntq3wk7lkmeyfhkp0wf5fgp49x5zpcnrn
        Legacy address: 		n1vho7tbggnYU3mCQgZHXzndh1RFawWDRo
        Taproot address:                bcrt1p07mls9jl9cmlg76q55x2e075r2j8uwykq9m9tlj45t5h7ggpjeks0tg49a
        Private key (WIF): 		n/a
        Extended private key (xprv):	n/a

But we need for mainnet also. Not just regtest.

asset/asset_test.go

coveralls · 2025-01-09T18:41:17Z

Pull Request Test Coverage Report for Build 12809434712

Details

0 of 0 changed or added relevant lines in 0 files are covered.
26 unchanged lines in 5 files lost coverage.
Overall coverage increased (+0.04%) to 40.762%

Files with Coverage Reduction	New Missed Lines	%
tapgarden/planter.go	2	71.02%
tapchannel/aux_leaf_signer.go	2	43.08%
tapdb/universe.go	4	80.91%
tapdb/multiverse.go	6	68.21%
universe/interface.go	12	51.95%

Totals
Change from base Build 12808880884:	0.04%
Covered Lines:	26553
Relevant Lines:	65142

💛 - Coveralls

ffranr · 2025-01-10T12:28:03Z

The base branch has not been merged yet. To avoid merging this PR prematurely, I will keep it in draft status. However, @ziggie1984, it is ready for review.

ffranr · 2025-01-10T12:32:58Z

Note also this comment from starius (from #1286):

Added more derivation paths and corresponding public keys, check in the test that the values returned by the method match to expected values. Also added tpub (testnet xpub).
To run "chantools derivekey" with xpubs, use PR lightninglabs/chantools#179

bhandras

LGTM 🎉

bhandras · 2025-01-10T12:51:17Z

asset/asset_test.go

+	testCases := []struct {
+		name          string
+		externalKey   ExternalKey
+		expectError   bool


nit: seems like there's no need for this bool, we can just expect an error if the expectedError is not empty.

Using the expectError bool flag we can specify that we expect an error without having to specify the error message. So I see it as optionality that we might as well keep. Let me know if you feel strongly about this and I can change as you suggest.

asset/asset_test.go

bhandras · 2025-01-10T14:58:40Z

asset/asset_test.go

+		t.Run(tc.name, func(tt *testing.T) {
+			pubKey, err := tc.externalKey.PubKey()
+
+			if tc.expectError {


nit: if tx.expectedError != ""

Nice not to have to specify the error message IMO, #1284 (comment)

asset/asset_test.go

ziggie1984

LGTM, tho I would say this change is so low level it almost feels it should be put into its own package, at least the taproot key generation tests.

GeorgeTsagk

Lgtm ✔️
-- pending comments by @bhandras

Roasbeef

tho I would say this change is so low level it almost feels it should be put into its own package

Would agree with this. This PR should be to the btcutil repo. It adds more tests to routines defined in the hdkeychain package.

Roasbeef

LGTM 🍐

ffranr · 2025-01-14T20:25:12Z

Moved to draft to avoid accidental merging before cold-group-key branch is merged.

ffranr · 2025-01-14T21:04:18Z

Needs rebase.

guggero · 2025-01-15T11:43:23Z

@ffranr shouldn't we just merge this, since it goes into the cold-group-key branch? Then we can still review it there and address comments? Two more commits shouldn't make that big of a difference :)

ffranr · 2025-01-15T22:17:57Z

@ffranr shouldn't we just merge this, since it goes into the cold-group-key branch? Then we can still review it there and address comments? Two more commits shouldn't make that big of a difference :)

At this point, cold-group-key branch has two approvals. Very close to a merge. I would prefer not to change it in anyway unless I have to.

guggero · 2025-01-16T12:29:40Z

Changed the base branch to main, needs a rebase.

This commit introduces a comprehensive unit test for the `PubKey` method of the `ExternalKey` struct. The test covers various scenarios, including valid and invalid derivation paths, ensuring the method's correctness and error handling.

Added more derivation paths and corresponding public keys, check in the test that the values returned by the method match to expected values. Also added tpub (testnet xpub). To run "chantools derivekey" with xpubs, use PR lightninglabs/chantools#179

ffranr self-assigned this Jan 9, 2025

ffranr mentioned this pull request Jan 9, 2025

Part 1: Asset Minting with V1 Asset Group Key and Chantools Cold Storage Support #1272

Merged

dstadulis added this to the v0.6 milestone Jan 9, 2025

ffranr commented Jan 9, 2025

View reviewed changes

asset/asset_test.go Outdated Show resolved Hide resolved

dstadulis requested a review from ziggie1984 January 9, 2025 14:59

dstadulis added testing unit-test labels Jan 9, 2025

dstadulis assigned starius Jan 9, 2025

ffranr marked this pull request as ready for review January 10, 2025 12:23

ffranr mentioned this pull request Jan 10, 2025

asset: add more test cases for ExternalKey.PubKey #1286

Closed

ffranr marked this pull request as draft January 10, 2025 12:26

ffranr requested a review from bhandras January 10, 2025 12:39

ffranr force-pushed the cold-group-key branch from e61d638 to c0c6795 Compare January 10, 2025 12:59

ffranr force-pushed the ExternalKey-PubKey-unit-test branch from c425baf to 94d8eba Compare January 10, 2025 13:04

bhandras approved these changes Jan 10, 2025

View reviewed changes

Roasbeef removed the request for review from ziggie1984 January 10, 2025 15:45

ziggie1984 reviewed Jan 13, 2025

View reviewed changes

ffranr force-pushed the cold-group-key branch from c0c6795 to 5299e8b Compare January 14, 2025 17:10

ffranr force-pushed the ExternalKey-PubKey-unit-test branch from 94d8eba to f5d4327 Compare January 14, 2025 17:37

ffranr requested a review from GeorgeTsagk January 14, 2025 17:49

GeorgeTsagk approved these changes Jan 14, 2025

View reviewed changes

Roasbeef requested changes Jan 14, 2025

View reviewed changes

Roasbeef marked this pull request as ready for review January 14, 2025 19:14

Roasbeef approved these changes Jan 14, 2025

View reviewed changes

ffranr marked this pull request as draft January 14, 2025 20:24

ffranr force-pushed the cold-group-key branch from 5299e8b to 49c2141 Compare January 14, 2025 20:58

ffranr force-pushed the cold-group-key branch from 49c2141 to c4a9222 Compare January 15, 2025 10:49

ffranr force-pushed the cold-group-key branch from c4a9222 to b801f9f Compare January 15, 2025 22:04

ffranr force-pushed the cold-group-key branch from b801f9f to 5ca4168 Compare January 16, 2025 12:01

guggero changed the base branch from cold-group-key to main January 16, 2025 12:29

ffranr and others added 2 commits January 16, 2025 12:56

ffranr force-pushed the ExternalKey-PubKey-unit-test branch from f5d4327 to cba1ca7 Compare January 16, 2025 13:00

ffranr marked this pull request as ready for review January 16, 2025 13:00

ffranr enabled auto-merge January 16, 2025 13:01

ffranr added this pull request to the merge queue Jan 16, 2025

Merged via the queue into main with commit 2e343d7 Jan 16, 2025
18 checks passed

guggero deleted the ExternalKey-PubKey-unit-test branch January 16, 2025 14:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add unit test for method ExternalKey.PubKey #1284

Add unit test for method ExternalKey.PubKey #1284

ffranr commented Jan 9, 2025 •

edited

Loading

ffranr commented Jan 9, 2025

ffranr commented Jan 9, 2025 •

edited

Loading

coveralls commented Jan 9, 2025 •

edited

Loading

ffranr commented Jan 10, 2025

ffranr commented Jan 10, 2025

bhandras left a comment

bhandras Jan 10, 2025

ffranr Jan 16, 2025

bhandras Jan 10, 2025

ffranr Jan 16, 2025

ziggie1984 left a comment

GeorgeTsagk left a comment

Roasbeef left a comment

Roasbeef left a comment

ffranr commented Jan 14, 2025

ffranr commented Jan 14, 2025

guggero commented Jan 15, 2025

ffranr commented Jan 15, 2025

guggero commented Jan 16, 2025

Add unit test for method ExternalKey.PubKey #1284

Add unit test for method ExternalKey.PubKey #1284

Conversation

ffranr commented Jan 9, 2025 • edited Loading

Notes for Reviewers

ffranr commented Jan 9, 2025

ffranr commented Jan 9, 2025 • edited Loading

coveralls commented Jan 9, 2025 • edited Loading

Pull Request Test Coverage Report for Build 12809434712

Details

💛 - Coveralls

ffranr commented Jan 10, 2025

ffranr commented Jan 10, 2025

bhandras left a comment

Choose a reason for hiding this comment

bhandras Jan 10, 2025

Choose a reason for hiding this comment

ffranr Jan 16, 2025

Choose a reason for hiding this comment

bhandras Jan 10, 2025

Choose a reason for hiding this comment

ffranr Jan 16, 2025

Choose a reason for hiding this comment

ziggie1984 left a comment

Choose a reason for hiding this comment

GeorgeTsagk left a comment

Choose a reason for hiding this comment

Roasbeef left a comment

Choose a reason for hiding this comment

Roasbeef left a comment

Choose a reason for hiding this comment

ffranr commented Jan 14, 2025

ffranr commented Jan 14, 2025

guggero commented Jan 15, 2025

ffranr commented Jan 15, 2025

guggero commented Jan 16, 2025

ffranr commented Jan 9, 2025 •

edited

Loading

ffranr commented Jan 9, 2025 •

edited

Loading

coveralls commented Jan 9, 2025 •

edited

Loading