Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

address: Address type and encoding #55

Merged
merged 1 commit into from
Jul 14, 2022
Merged

address: Address type and encoding #55

merged 1 commit into from
Jul 14, 2022

Conversation

jharveyb
Copy link
Collaborator

@jharveyb jharveyb commented Jul 6, 2022

Split from #43. Adds tests.

Currently have a test failure for the TLV encode + decode round trip, with the ScriptKey and InternalKey fields. I think it's related to those fields being pubkey values and not pointers, but not sure.

=== CONT  TestAddressEncoding
    /home/jhb/taro/taro/address/address_test.go:226:
                Error Trace:    address_test.go:78
                                                        address_test.go:172
                                                        address_test.go:226
                Error:          Not equal:
                                expected: secp256k1.PublicKey{x:secp256k1.FieldVal{n:[10]uint32{0x30571be, 0x1322145, 0x1d0fa14, 0x7d489a, 0x946188, 0x239116, 0x3bd77a4, 0x13588c5, 0x2d52456, 0x2d6238}}, y:secp256k1.FieldVal{n:[10]uint32{0x2eb9e12, 0xa5e12e, 0x6c6750, 0x3e06e8e, 0x5e90f7, 0x3306d49, 0x369c176, 0x25d7b1, 0x21c75d3, 0x23ce57}}}
                                actual  : secp256k1.PublicKey{x:secp256k1.FieldVal{n:[10]uint32{0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0}}, y:secp256k1.FieldVal{n:[10]uint32{0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0}}}

                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -3,12 +3,12 @@
                                   n: ([10]uint32) (len=10) {
                                -   (uint32) 50688446,
                                -   (uint32) 20062533,
                                -   (uint32) 30472724,
                                -   (uint32) 8210586,
                                -   (uint32) 9724296,
                                -   (uint32) 2330902,
                                -   (uint32) 62748580,
                                -   (uint32) 20285637,
                                -   (uint32) 47522902,
                                -   (uint32) 2974264
                                +   (uint32) 0,
                                +   (uint32) 0,
                                +   (uint32) 0,
                                +   (uint32) 0,
                                +   (uint32) 0,
                                +   (uint32) 0,
                                +   (uint32) 0,
                                +   (uint32) 0,
                                +   (uint32) 0,
                                +   (uint32) 0
                                   }
                                @@ -17,12 +17,12 @@
                                   n: ([10]uint32) (len=10) {
                                -   (uint32) 48995858,
                                -   (uint32) 10871086,
                                -   (uint32) 7104336,
                                -   (uint32) 65040014,
                                -   (uint32) 6197495,
                                -   (uint32) 53505353,
                                -   (uint32) 57262454,
                                -   (uint32) 2480049,
                                -   (uint32) 35419603,
                                -   (uint32) 2346583
                                +   (uint32) 0,
                                +   (uint32) 0,
                                +   (uint32) 0,
                                +   (uint32) 0,
                                +   (uint32) 0,
                                +   (uint32) 0,
                                +   (uint32) 0,
                                +   (uint32) 0,
                                +   (uint32) 0,
                                +   (uint32) 0
                                   }
                Test:           TestAddressEncoding

Fixes #15

@Roasbeef
Copy link
Member

Roasbeef commented Jul 6, 2022

                            actual  : secp256k1.PublicKey{x:secp256k1.FieldVal{n:[10]uint32{0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0}}, y:secp256k1.FieldVal{n:[10]uint32{0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0}}}

Looks like the issue is that this one is empty? There's also a slight quirk w/ how things are: with the family key we store the raw key, but only serialize the tweaked key. We'll likely need to make a custom comparison function.

@Roasbeef Roasbeef added the addr label Jul 6, 2022
@jharveyb
Copy link
Collaborator Author

jharveyb commented Jul 6, 2022

Looks like the issue is that this one is empty? There's also a slight quirk w/ how things are: with the family key we store the raw key, but only serialize the tweaked key. We'll likely need to make a custom comparison function.

Seems like addresses should be including only the tweaked key? That's the only part of the FamilyKey struct i imagine would be public or relevant for receiver addresses.

address/address.go Outdated Show resolved Hide resolved
address/address.go Outdated Show resolved Hide resolved
address/address.go Outdated Show resolved Hide resolved
address/address.go Show resolved Hide resolved
address/address.go Outdated Show resolved Hide resolved
address/address.go Show resolved Hide resolved
address/address.go Outdated Show resolved Hide resolved
address/address_test.go Outdated Show resolved Hide resolved
)
}

func NewAddressScriptKeyRecord(scriptKey *btcec.PublicKey) tlv.Record {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So FWIW, we already have some encode/decode methods for things like these in the existing asset package. I think the only reason not to use those, is if we end up w/ some weird import cycle down the line.

@Roasbeef
Copy link
Member

Roasbeef commented Jul 9, 2022

Here's the fix I needed to throw in the proof PR to get the tests passing again: 74cc870#diff-0444dc75ab5f91d776dfc97ff7a8de031c33a2a5c25c0a488b4896ba617b53aaR159

The issue is we have a field in the FamilyKey struct that actually isn't ever encoded on the wire, so when you compare the two versions the in-memory one has it set but the other doesn't.

@dstadulis
Copy link
Collaborator

Updating coming later today then JHB needs review

@jharveyb
Copy link
Collaborator Author

Updated to address feedback, use a Taro-specific params struct for handling address HRP, and fix the PubKey encode/decode bug.

Unsure if having this private schnorrPubKeyEncode/Decode is the best option; I imagine we'd want to use this elsewhere. Could adjust naming to better distinguish from the version in the asset package.

@Roasbeef
Copy link
Member

Unsure if having this private schnorrPubKeyEncode/Decode is the best option; I imagine we'd want to use this elsewhere. Could adjust naming to better distinguish from the version in the asset package.

Yeah I think ultimately we'll aggregate these into some internal encoding package.

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🍬

@Roasbeef Roasbeef merged commit c3bffba into main Jul 14, 2022
@guggero guggero deleted the address-type branch July 15, 2022 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

bip-taro-addr: create initial implementation of the taro address format
3 participants