Jörmungandr: Separate concern between keyToAddress and get/putAddress #417

Anviking · 2019-06-16T21:19:40Z

Issue Number

#219 - or bug

Overview

Single addresses contain a header byte and a public key (32 bytes). I believe we should store the entire 33 bytes inside the Address data-type. Previously didn't store the header byte.

I have made getAddress and putAddress work preserve the entire address structure.
- Previously:
  - getAddress :: 33 bytes -> Address 32 bytes
  - putAddress :: Address 32 bytes -> 33 bytes
- Now:
  - getAddress :: 33 bytes -> Address 33 bytes
  - putAddress Address 33 bytes -> 33 bytes
I have implemented keyToAddress :: XPub 32 bytes -> Address 33 bytes
I renamed Address {getAddress} to Address {unAddress} to avoid name collisions with Binary.getAddress

Comments

Anviking · 2019-06-16T22:44:36Z

lib/jormungandr/test/unit/Cardano/Wallet/Jormungandr/BinarySpec.hs

@@ -102,7 +102,7 @@ genesisBlock = Block genesisHeader
        { inputs = []
        , outputs =
            [ TxOut
-                { address = Address "3$\195xi\193\"h\154\&5\145}\245:O\"\148\163\165/h^\ENQ\245\248\229;\135\231\234E/"
+                { address = Address "\131\&3$\195xi\193\"h\154\&5\145}\245:O\"\148\163\165/h^\ENQ\245\248\229;\135\231\234E/"


The header byte

rvl

👍

rvl · 2019-06-17T05:04:17Z

lib/jormungandr/src/Cardano/Wallet/Jormungandr/Compatibility.hs

-instance KeyToAddress (Jormungandr n) where
-    keyToAddress = undefined
+instance forall n. KnownNetwork n => KeyToAddress (Jormungandr n) where
+    keyToAddress key = Address


Suggested change

keyToAddress key = Address

keyToAddress = Address . addressToByteString . getKey

I would prefer this to be encapsulated in a function which lives in the Binary module. So that the Put stuff doesn't leak out. Something like:

addressToByteString :: XPub -> ByteString addressToByteString = BL.toStrict . runPut . putSingleAddress

Up to you though.

Agreed with @rvl here 👍

I went with

singleAddressFromKey :: forall n. KnownNetwork n => Proxy n -> XPub -> Address singleAddressFromKey _ xpub = Address $ BL.toStrict $ runPut $ do putWord8 (single @n) putByteString (unXPub xpub)

--> 5bc2157

Considered naming it something like newSingleAddress too.

rvl · 2019-06-17T05:07:18Z

lib/core/src/Cardano/Wallet/Primitive/Types.hs

@@ -527,7 +527,7 @@ isPending = (== Pending) . (status :: TxMeta -> TxStatus) . snd
 -- layer and that the underlying encoding is rather agnostic to the underlying
 -- backend.
 newtype Address = Address
-    { getAddress :: ByteString
+    { unAddress :: ByteString


I do like unAddress better than getAddress.

However there is always the option of name qualification or import hiding if we want to keep the newtype accessors named consistently.

KtorZ · 2019-06-17T09:56:27Z

lib/jormungandr/src/Cardano/Wallet/Jormungandr/Binary.hs

+    -- be included in the underlying Address ByteString.
+    headerByte <- label "address header" . lookAhead $ getWord8
+    let kind = kindValue headerByte
+    let _discrimination = discriminationValue headerByte


What's the use for discriminationValue here 🤔 ?

Nothing… Unless we would like to check that it matches, but as we get the blocks (including genesis) from the node, that would probably not be worth it

KtorZ · 2019-06-17T09:57:15Z

lib/jormungandr/src/Cardano/Wallet/Jormungandr/Binary.hs

+    case kind of
+        -- Single Address
+        0x3 -> Address <$> getByteString 33
+        0x4 -> error "unimplemented group address decoder"


How big of an effort is it to add support for group address? I expect it to be only Address <$> getByteString 65 ?

I suppose that will just work…

We won't be able to discover them in isOurs though as keyToAddress only can create one type of addresses (now single).

True although the concern is slightly different here. BIP-44 for group addresses is out of the question for now. Support for delegation and delegation addresses will come later.

KtorZ · 2019-06-17T09:59:36Z

lib/jormungandr/src/Cardano/Wallet/Jormungandr/Compatibility.hs

-instance KeyToAddress (Jormungandr n) where
-    keyToAddress = undefined
+instance forall n. KnownNetwork n => KeyToAddress (Jormungandr n) where
+    keyToAddress key = Address


Agreed with @rvl here 👍

Anviking · 2019-06-17T10:33:04Z

lib/jormungandr/src/Cardano/Wallet/Jormungandr/Binary.hs

+    -- We use 'lookAhead' to not consume the header, and let it
+    -- be included in the underlying Address ByteString.
+    headerByte <- label "address header" . lookAhead $ getWord8
+    let kind = kindValue headerByte


Also, @KtorZ, there is difference in style here with the binary arithmetic:

case kind of -- Single Address 0x3 -> Address <$> getByteString 33 0x4 -> error "unimplemented group address decoder" 0x5 -> error "unimplemented account address decoder" 0x6 -> error "unimplemented multisig address decoder" other -> fail $ "Invalid address type: " ++ show other where kindValue :: Word8 -> Word8 kindValue = (.&. 0b01111111) discriminationValue :: Word8 -> Network discriminationValue b = case b .&. 0b10000000 of 0 -> Mainnet _ -> Testnet

vs hard-coded single/grouped values specific to each network found in Cardano.Wallet.Jormungandr.Environment

instance KnownNetwork 'Mainnet where networkVal = Mainnet hrp = unsafeHumanReadablePart "ca" single = 0x03 grouped = 0x04 instance KnownNetwork 'Testnet where networkVal = Testnet hrp = unsafeHumanReadablePart "ta" single = 0x83 grouped = 0x84

I'm not sure what should be done. Do we really want to throw the arithmetic away?

It's not about throwing it away and it's not about using hard-coded bytes or extracting them from the binary data. It's about checking that both matches. That's the only reason to have a discriminant version, to make sure that we do not use addresses on the wrong network.

We do already perform this check as part of the encode/decode functions since this is our entry-point for user-facing data. We could also do it in the binary modules when parsing blocks although we do expect the underlying node to do that for us already.

paweljakubas

LGTM

akegalj

lgtm

akegalj · 2019-06-17T12:02:11Z

lib/jormungandr/src/Cardano/Wallet/Jormungandr/Binary.hs

@@ -267,10 +276,6 @@ putTokenTransfer _ (Tx inputs outputs) = do
    putOutput (TxOut address coin) = do
        putAddress address
        putWord64be $ getCoin coin
-    putAddress address = do
-        -- NOTE: only single address supported for now
-        putWord8 (single @n)


yes - I was not happy to introduce new constrain just to resolve the network here. Having this bit in address is much more elegant solution

akegalj · 2019-06-17T12:05:50Z

lib/jormungandr/test/unit/Cardano/Wallet/Jormungandr/BinarySpec.hs

@@ -102,7 +102,7 @@ genesisBlock = Block genesisHeader
        { inputs = []
        , outputs =
            [ TxOut
-                { address = Address "3$\195xi\193\"h\154\&5\145}\245:O\"\148\163\165/h^\ENQ\245\248\229;\135\231\234E/"
+                { address = Address "\131\&3$\195xi\193\"h\154\&5\145}\245:O\"\148\163\165/h^\ENQ\245\248\229;\135\231\234E/"


Anviking · 2019-06-17T14:02:13Z

lib/jormungandr/test/unit/Cardano/Wallet/Jormungandr/BinarySpec.hs

+        it "encodes (network <> tag <> key) correctly into an address" $
+            singleAddressFromKey (Proxy @'Mainnet) pub
+            `shouldBe`
+            Address (BS.pack [3] <> pub)


I'm thinking we can these simple cases here, and test keyToAddress more extensively together comparing with jcli

Where does the 3 come from here?

let singleAddressOnMainnet = 3

lib/jormungandr/src/Cardano/Wallet/Jormungandr/Environment.hs

KtorZ · 2019-06-17T14:27:50Z

lib/jormungandr/src/Cardano/Wallet/Jormungandr/Binary.hs

@@ -454,15 +452,26 @@ putAddress addr@(Address bs)
        ++ ": " ++ show addr
  where l = BS.length bs

-singleAddressFromKey :: forall n. KnownNetwork n => Proxy n -> XPub -> Address
-singleAddressFromKey _ xpub = Address $ BL.toStrict $ runPut $ do
+singleAddressFromKey :: forall n. KnownNetwork n => Proxy n -> ByteString -> Address


The type was better before using the XPub. What's the rationale for choosing a raw ByteString instead?

We don't have a ChainCode.

Yes, but we can extract the relevant bits from within singleAddressFromKey. For lack of a better intermediate type, XPub is cleaner than a raw ByteString

lib/jormungandr/src/Cardano/Wallet/Jormungandr/Compatibility.hs

KtorZ · 2019-06-17T14:30:58Z

lib/jormungandr/test/unit/Cardano/Wallet/Jormungandr/BinarySpec.hs

+            Address (BS.pack [3] <> pub)
+        it "throws when length (key) != 32" $
+            evaluate (singleAddressFromKey (Proxy @'Mainnet) (pub <> "\148"))
+            `shouldThrow` anyErrorCall


Ho. Is this the behavior from put & isolate ?

isolate is for Get. I added a isolatePut.

Also, we should probably ban the unsafe runPut/runGet for safer ones soon.

to avoid naming collision with Binary.getAddress.

Previously: `getAddress :: ByteString 33 bytes -> Address 32 bytes` `putAddress :: Address 32 bytes -> ByteString 33 bytes` Now: `getAddress :: ByteString 33 bytes -> Address 33 bytes` `putAddress Address 33 bytes -> ByteString 33 bytes` **Implemented** `keyToAddress :: XPub 32 bytes -> Address 33 bytes` - Implement Jörmungandr keyToAddress - Refactor to have singleAddressFromKey in Binary module - Ignore chaincode in jörmungandr keyToAddress & add tests

Anviking self-assigned this Jun 16, 2019

Anviking force-pushed the anviking/219/fix-address-confusion branch from 817e92e to 62ed3fe Compare June 16, 2019 21:27

Anviking changed the title ~~Jörmungandr: Separate confusion between keyToAddress and get/putAddress~~ Jörmungandr: Separate concern between keyToAddress and get/putAddress Jun 16, 2019

Anviking force-pushed the anviking/219/fix-address-confusion branch 2 times, most recently from 6d26472 to 6ddaeff Compare June 16, 2019 22:14

Anviking requested review from akegalj, KtorZ and paweljakubas June 16, 2019 22:22

Anviking commented Jun 16, 2019

View reviewed changes

rvl approved these changes Jun 17, 2019

View reviewed changes

KtorZ reviewed Jun 17, 2019

View reviewed changes

Anviking commented Jun 17, 2019

View reviewed changes

Anviking force-pushed the anviking/219/fix-address-confusion branch from 173f5dd to 5bc2157 Compare June 17, 2019 11:04

paweljakubas approved these changes Jun 17, 2019

View reviewed changes

Anviking force-pushed the anviking/219/fix-address-confusion branch from af72402 to b8a0d3b Compare June 17, 2019 11:22

akegalj approved these changes Jun 17, 2019

View reviewed changes

Anviking force-pushed the anviking/219/fix-address-confusion branch from 5b8354b to 1c9fbed Compare June 17, 2019 13:55

Anviking commented Jun 17, 2019

View reviewed changes

KtorZ reviewed Jun 17, 2019

View reviewed changes

lib/jormungandr/src/Cardano/Wallet/Jormungandr/Environment.hs Outdated Show resolved Hide resolved

KtorZ reviewed Jun 17, 2019

View reviewed changes

lib/jormungandr/src/Cardano/Wallet/Jormungandr/Compatibility.hs Outdated Show resolved Hide resolved

KtorZ reviewed Jun 17, 2019

View reviewed changes

Anviking force-pushed the anviking/219/fix-address-confusion branch 3 times, most recently from 12863d7 to f585ce5 Compare June 18, 2019 09:08

Anviking added 4 commits June 18, 2019 14:08

Rename Address { getAddress} to Address { unAddress }

6cb83c1

to avoid naming collision with Binary.getAddress.

getTransaction has dynamic length and shouldn't use isolate

549eb8d

Enable decoding of grouped/account/multisig addresses

fef2557

KtorZ force-pushed the anviking/219/fix-address-confusion branch from f585ce5 to 429e310 Compare June 18, 2019 12:15

KtorZ force-pushed the anviking/219/fix-address-confusion branch from 429e310 to 3e23420 Compare June 18, 2019 12:17

better cache build dependencies in Travis

cd81207

KtorZ force-pushed the anviking/219/fix-address-confusion branch from 3e23420 to cd81207 Compare June 18, 2019 12:27

KtorZ merged commit 713cef8 into master Jun 18, 2019

KtorZ deleted the anviking/219/fix-address-confusion branch June 18, 2019 13:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jörmungandr: Separate concern between keyToAddress and get/putAddress #417

Jörmungandr: Separate concern between keyToAddress and get/putAddress #417

Anviking commented Jun 16, 2019 •

edited

Loading

Anviking Jun 16, 2019

akegalj Jun 17, 2019

rvl left a comment

rvl Jun 17, 2019

KtorZ Jun 17, 2019

Anviking Jun 17, 2019

rvl Jun 17, 2019

KtorZ Jun 17, 2019

Anviking Jun 17, 2019

KtorZ Jun 17, 2019

Anviking Jun 17, 2019

KtorZ Jun 17, 2019

KtorZ Jun 17, 2019

Anviking Jun 17, 2019

KtorZ Jun 17, 2019

paweljakubas left a comment

akegalj left a comment

akegalj Jun 17, 2019

akegalj Jun 17, 2019

Anviking Jun 17, 2019

KtorZ Jun 17, 2019 •

edited

Loading

Anviking Jun 17, 2019

KtorZ Jun 17, 2019

Anviking Jun 17, 2019

KtorZ Jun 17, 2019

KtorZ Jun 17, 2019

Anviking Jun 17, 2019

	keyToAddress key = Address
	keyToAddress = Address . addressToByteString . getKey

Jörmungandr: Separate concern between keyToAddress and get/putAddress #417

Jörmungandr: Separate concern between keyToAddress and get/putAddress #417

Conversation

Anviking commented Jun 16, 2019 • edited Loading

Issue Number

Overview

Comments

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paweljakubas left a comment

Choose a reason for hiding this comment

akegalj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KtorZ Jun 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anviking commented Jun 16, 2019 •

edited

Loading

KtorZ Jun 17, 2019 •

edited

Loading