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.

Sign up for GitHub

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

Jump to bottom

Move {Encode,Decode}Address within backend-specific package #1677

Merged

iohk-bors merged 7 commits into master from KtorZ/move-encode-decode-address

May 22, 2020

Member

KtorZ commented May 20, 2020

Issue Number

Overview

6845b48
📍 move Aeson and HttpApiData roundtrip functions to test-utils
bc83f45
📍 relocate {Encode,Decode}Address instances to backend-specific packages
So that each package can actually define how addresses should be parsed from the API, and provide different rules for decoding addresses for Byron, Jormungandr and Shelley.
STILL TODO: encoding / decoding in Shelley (which should be similar to approach used in Jörmungandr, but with Shelley address decoders instead).

Comments

Big diff as I had to re-generate a lot of the golden test files for the API since the address encoding now depends on the chosen backend. This consequently fixes an "unreported issue" where Jörmungandr addresses would be accepted by cardano-wallet-byron despite being totally invalid.

Also, I re-located many tests in backend-specific package if they were actually testing things related to the encoding.

KtorZ self-assigned this

KtorZ commented

View reviewed changes

lib/byron/test/unit/Cardano/Wallet/Byron/CompatibilitySpec.hs

@@ @@ -60,6 +94,109 @@ spec = do @@
                           let toPoint' = toPoint gh epochLength
                           toPoint' (fromTip' tip) === (getTipPoint tip)
+                  describe "Hardware Ledger" $ do

Member Author

KtorZ May 20, 2020

Moved from Cardano.Wallet.Primitive.AddressDerivation.IcarusSpec

KtorZ commented

View reviewed changes

lib/byron/test/unit/Cardano/Wallet/Byron/CompatibilitySpec.hs

+                          , "Ae2tdPwUPEZLU4TEkPMmkT2dfQ23YyKLFWXBwuxLi4rxF4kT6najwAi6APQ"
+                          , "Ae2tdPwUPEZBFKNnz2F1Bn5pLkhp2rm9byDAyW1JzN7ZUYSgRPqrH3Jgs88"
+                          ]

Member Author

KtorZ May 20, 2020

Moved from Cardano.Wallet.Primitive.AddressDerivation.IcarusSpec

KtorZ commented

View reviewed changes

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

@@ @@ -1425,77 +1412,6 @@ class EncodeAddress (n :: NetworkDiscriminant) where @@
               class DecodeAddress (n :: NetworkDiscriminant) where
                   decodeAddress :: Text -> Either TextDecodingError Address
-              -- | Encode an 'Address' to a human-readable format. This produces two kinds of

Member Author

KtorZ May 20, 2020

Moved to Cardano.Wallet.Jormungandr.Compatibility

KtorZ commented

View reviewed changes

lib/core/test/unit/Cardano/Wallet/Api/Malformed.hs

-                      [ ( "NOT-AN-ADDRESS"
-                        , "Unable to decode Address: encoding is neither Bech32 nor Base58."
-                        )
-                      ]

Member Author

KtorZ May 20, 2020

The generic malformed tests are no longer able to test errors related to the address encoding since it now depends on the underlying backend. So, negative tests are done in backend-specific tests suites on a per-case base .

KtorZ commented

View reviewed changes

lib/core/test/unit/Cardano/Wallet/Api/TypesSpec.hs

@@ @@ -383,8 +340,6 @@ spec = do @@
                           httpApiDataRoundtrip $ Proxy @(ApiT AddressState)
                           httpApiDataRoundtrip $ Proxy @Iso8601Time
                           httpApiDataRoundtrip $ Proxy @(ApiT SortOrder)
-                          httpApiDataRoundtrip $ Proxy @(ApiT Address, Proxy 'Mainnet)
-                          httpApiDataRoundtrip $ Proxy @(ApiT Address, Proxy ('Testnet 0))

Member Author

KtorZ May 20, 2020

Moved from Cardano.Wallet.Jormungandr.CompatibilitySpec

KtorZ commented

View reviewed changes

lib/core/test/unit/Cardano/Wallet/Api/TypesSpec.hs

@@ @@ -310,7 +268,6 @@ spec = do @@
                   describe
                       "can perform roundtrip JSON serialization & deserialization, \
                       \and match existing golden files" $ do
-                          jsonRoundtripAndGolden $ Proxy @(ApiAddress ('Testnet 0))

Member Author

KtorZ May 20, 2020 •

edited

Loading

Moved to Cardano.Wallet.Jormungandr.CompatibilitySpec

KtorZ commented

View reviewed changes

lib/core/test/unit/Cardano/Wallet/Api/TypesSpec.hs

-                          let msg = "Error in $: Unable to decode Address: \
-                                  \encoding is neither Bech32 nor Base58."
-                          Aeson.parseEither parseJSON [aesonQQ|"-----"|]
-                              `shouldBe` (Left @String @(ApiT Address, Proxy ('Testnet 0)) msg)

Member Author

KtorZ May 20, 2020 •

edited

Loading

Moved to Cardano.Wallet.Jormungandr.CompatibilitySpec

KtorZ commented

View reviewed changes

lib/core/test/unit/Cardano/Wallet/Api/TypesSpec.hs

@@ @@ -569,130 +518,7 @@ spec = do @@
                           parseUrlPiece "patate"
                               `shouldBe` (Left @Text @(ApiT AddressState) msg)
-                  describe "encodeAddress & decodeAddress (Mainnet)" $ do

Member Author

KtorZ May 20, 2020 •

edited

Loading

Moved to Cardano.Wallet.Jormungandr.CompatibilitySpec

KtorZ commented

View reviewed changes

lib/core/test/unit/Cardano/Wallet/Api/TypesSpec.hs

-                          "addr1qn0e7zr89gafgauz9xu3m25cz5ugs0s4xhtxdhqsuca58r6ycclr\
-                          \7sp2hlmqvhyywy266ghldvxn4p0adxn0esew6a423jkmxpdsc5d8n8hz07"
-                  describe "encodeAddress & decodeAddress (Testnet)" $ do

Member Author

KtorZ May 20, 2020 •

edited

Loading

Moved to Cardano.Wallet.Jormungandr.CompatibilitySpec

KtorZ commented

View reviewed changes

lib/core/test/unit/Cardano/Wallet/Api/TypesSpec.hs

-              -- new format. Faulty golden files should /not/ be commited.
-              --
-              -- The directory `test/data/Cardano/Wallet/Api` is used.
-              jsonRoundtripAndGolden

Member Author

KtorZ May 20, 2020

Extracted in test-utils to be available from multiple packages.

Contributor

paweljakubas May 22, 2020

good move

KtorZ commented

View reviewed changes

lib/core/test/unit/Cardano/Wallet/Api/TypesSpec.hs

-                  -> [ByteString]
-                  -> Text
-                  -> SpecWith ()
-              goldenTestAddr _proxy pubkeys expected = it ("golden test: " <> T.unpack expected) $ do

Member Author

KtorZ May 20, 2020

extracted in test-utils to be available from multiple packages

KtorZ commented

View reviewed changes

lib/core/test/unit/Cardano/Wallet/Api/TypesSpec.hs

-                          ]
-                      return (addr, Proxy)
-              instance {-# OVERLAPS #-} Arbitrary (ApiT Address, Proxy ('Testnet 0)) where

Member Author

KtorZ May 20, 2020

instances moved to Cardano.Wallet.Jormungandr.CompatibilitySpec

KtorZ commented

View reviewed changes

lib/core/test/unit/Cardano/Wallet/Gen.hs

+              genLegacyAddress
+                  :: Maybe ProtocolMagic
+                  -> Gen Address
+              genLegacyAddress pm = do

Member Author

KtorZ May 20, 2020

Moved from Cardano.Wallet.Primitive.AddressDerivationSpec for easier reuse

KtorZ commented

View reviewed changes

lib/core/test/unit/Cardano/Wallet/Primitive/AddressDerivation/IcarusSpec.hs

               spec :: Spec
               spec = do
-                  describe "Golden Tests - Icarus' style addresses" $ do

Member Author

KtorZ May 20, 2020

Moved to Cardano.Wallet.Byron.CompatibilitySpec

KtorZ commented

View reviewed changes

lib/core/test/unit/Cardano/Wallet/Primitive/AddressDerivation/IcarusSpec.hs

@@ @@ -119,154 +71,6 @@ spec = do @@
                       it "paymentKeyFingerprint . liftPaymentAddress == pure" $
                           property prop_roundtripFingerprintLift
-                  describe "Hardware Ledger" $ do

Member Author

KtorZ May 20, 2020

Moved to Cardano.Wallet.Byron.CompatibilitySpec

KtorZ commented

View reviewed changes

lib/core/test/unit/Cardano/Wallet/Primitive/AddressDerivationSpec.hs

@@ @@ -10,16 +10,12 @@ @@
               module Cardano.Wallet.Primitive.AddressDerivationSpec
                   ( spec
-                  -- * Generators
-                  , genAddress

Member Author

KtorZ May 20, 2020

Moved to Cardano.Wallet.Gen

KtorZ commented

View reviewed changes

lib/core/test/unit/Cardano/Wallet/Primitive/AddressDerivationSpec.hs

-                  -- * Generators
-                  , genAddress
-                  , genLegacyAddress

Member Author

KtorZ May 20, 2020

Moved to Cardano.Wallet.Jormungandr.CompatibilitySpec

KtorZ commented

View reviewed changes

lib/core/test/unit/Cardano/Wallet/Primitive/AddressDerivationSpec.hs

    
              import qualified Cardano.Wallet.Primitive.AddressDerivation.Jormungandr as Seq

              import qualified Cardano.Wallet.Primitive.AddressDerivation.Byron as Byron

              import qualified Cardano.Wallet.Primitive.AddressDerivation.Icarus as Icarus

              import qualified Cardano.Wallet.Primitive.AddressDerivation.Jormungandr as Jormungandr

Member Author

KtorZ May 20, 2020

Meh.

KtorZ commented

View reviewed changes

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

+              {-------------------------------------------------------------------------------
+                                    Address Encoding / Decoding
+              -------------------------------------------------------------------------------}

Member Author

KtorZ May 20, 2020

👇 Moved from Cardano.Wallet.Api.Types

Contributor

paweljakubas May 22, 2020

good good

KtorZ commented

View reviewed changes

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

               spec :: Spec
-              spec = pure ()
+              spec = do

Member Author

KtorZ May 20, 2020

👇 Moved from Cardano.Wallet.Api.TypesSpec

KtorZ requested a review from Anviking

May 20, 2020 16:18

KtorZ added the ADDING FEATURE label

KtorZ marked this pull request as ready for review

May 22, 2020 08:57

KtorZ and others added 3 commits

May 22, 2020 11:14


          move Aeson and HttpApiData roundtrip functions to test-utils

269fb44


          relocate {Encode,Decode}Address instances to backend-specific packages

f9ce183

So that each package can actually define how addresses should be parsed from the API, and provide different rules for decoding addresses for Byron, Jormungandr and Shelley.


          Regenerate nix

19e81e7

paweljakubas approved these changes

View reviewed changes

Contributor

paweljakubas left a comment

logical address-related code reshuffling (would be easier to decouple jormungandr as separate repo in the future thanks to that). Moves specific backend code from core to backends directories. My only question is : why all lib/core/test/data/Cardano/Wallet/Api/* was regenerated and included in this PR ?

KtorZ added 2 commits

May 22, 2020 12:29


          take custom json golden test dir as argument (and re-generate golden)

d3c3666

So that the template haskell splice 'getTestDataDir' can be evaluated within the context of the package actually running the test, instead of the context of 'test-utils'.


          remove redundant weeder dependencies

f2a8ff0

KtorZ force-pushed the KtorZ/move-encode-decode-address branch from 5656699 to f2a8ff0 Compare

May 22, 2020 10:29


          fixup! Regenerate nix

df9cbd3

Member Author

KtorZ commented May 22, 2020

bors r+

iohk-bors bot added a commit that referenced this pull request


          Merge #1677

ca554c2

1677: Move {Encode,Decode}Address within backend-specific package r=KtorZ a=KtorZ

# Issue Number

<!-- Put here a reference to the issue this PR relates to and which requirements it tackles -->

#1672 

# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- 6845b48
  📍 **move Aeson and HttpApiData roundtrip functions to test-utils**
  
- bc83f45
  📍 **relocate {Encode,Decode}Address instances to backend-specific packages**
  So that each package can actually define how addresses should be parsed from the API, and provide different rules for decoding addresses for Byron, Jormungandr and Shelley.

- STILL TODO: encoding / decoding in Shelley (which should be similar to approach used in Jörmungandr, but with Shelley address decoders instead).

# Comments

<!-- Additional comments or screenshots to attach if any -->

Big diff as I had to re-generate a lot of the golden test files for the API since the address encoding now depends on the chosen backend. This consequently fixes an "unreported issue" where Jörmungandr addresses would be accepted by cardano-wallet-byron despite being totally invalid. 

Also, I re-located many tests in backend-specific package if they were actually testing things related to the encoding.

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Assign the PR to a corresponding milestone
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: KtorZ <[email protected]>
Co-authored-by: IOHK <[email protected]>

Contributor

iohk-bors bot commented May 22, 2020

Build failed

buildkite/cardano-wallet


          adjust new error message in test case

2d8afe9

Byron addresses have no reason to mention bech32 actually

Member Author

KtorZ commented May 22, 2020

bors r+

Contributor

iohk-bors bot commented May 22, 2020

Build succeeded

iohk-bors bot merged commit ced9ed1 into master

iohk-bors bot deleted the KtorZ/move-encode-decode-address branch

May 22, 2020 16:00

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels