EOSWALLETS tests

[piotr-iohk]

#

Overview

• I have added tests for EOSWALLETS_CREATE capability -> https://docs.google.com/spreadsheets/d/1ztF2PnnrMnkAxrhdq3LUbP1KazxcxuMrbiObTs1CuTk
• I have added few tests covering read, update, delete and list capability
• I intend to add more tests for EOSWALLETS here

Comments

[piotr-iohk]

I like this error message Address pool gap should be in range [10..100]. I was expecting it also when providing -1 as addressPoolGap. Instead we have Word8 is either floating or will cause over or underflow: -1.0. Maybe we'd like to change it for consistency?

[akegalj]

Error in $.addressPoolGap: Word8 is either floating or will cause over or underflow: -1.0

msg is interesting one - I didn't expect to see something like this. For example when I try to manually create address pool gap with a negative number:

> AddressPoolGap (-1)

I get

    Literal -1 is out of the Word8 range 0..255

which makes sense as Word8 is defined in specified range.

Edit:
Aha - msg comes from Aeson and decoding a "-1" string into a Word8:

> import Data.Aeson
> eitherDecode "-1" :: Either String Word8
Left "Error in $: Word8 is either floating or will cause over or underflow: -1.0"

so everything is as expected here. So you will try to push your json to the api. First thing that api will do is it will try to decode this json into expected Haskell data type. This will immediatelly fail in this json decoding part as Word8 won't be able to decode "-1" and Aeson library will rise the exception. If the exception wasn't rased by underlying aeson lib, we would catch it and rise "Address pool gap should be in range [10..100]".

If we would like to rise only single exception for AddressPoolGap one could define custom FromJSON instance that works only for [10..100] (ie, [minBound..maxBound]) and not to use custom/generic aeson instance https://github.com/input-output-hk/cardano-wallet/blob/15dd91b05112c16be506129e5c00e88114f8bd16/src/Cardano/Wallet/Kernel/AddressPoolGap.hs#L33 . This would do the trick in our case:

instance FromJSON AddressPoolGap where
    parseJSON = parseJSON >=> \gap -> case mkAddressPoolGap gap of
        Left err -> fail $ toString $ sformat build $ bprint
        ("Address pool gap should be in range ["%int%".."%int%"].")
        (minBound @AddressPoolGap)
        (maxBound @AddressPoolGap)
        Right x  -> pure x

or something along these lines. Anyway if you think this would be nice to have please open a new ticket and we will discuss it with rest of the team. For now I don't think this is necessary nor urgent to have

[piotr-iohk]

@denisshevchenko - I'll ask here as it is related to creating EOS wallets. In the API doc -> https://input-output-hk.github.io/cardano-wallet/#tag/Externally-Owned-Wallets%2Fpaths%2F~1api~1v1~1wallets~1externally-owned%2Fpost it is said Creates a new or restores an existing Wallet. Is there an option to restore or the mention about the restore should be actually removed from the doc?

[denisshevchenko]

Is there an option to restore

@piotr-iohk Not yet.

[akegalj]

Error in $.addressPoolGap: Word8 is either floating or will cause over or underflow: -1.0

msg is interesting one - I didn't expect to see something like this. For example when I try to manually create address pool gap with a negative number:

> AddressPoolGap (-1)

I get

    Literal -1 is out of the Word8 range 0..255

which makes sense as Word8 is defined in specified range.

Edit:
Aha - msg comes from Aeson and decoding a "-1" string into a Word8:

> import Data.Aeson
> eitherDecode "-1" :: Either String Word8
Left "Error in $: Word8 is either floating or will cause over or underflow: -1.0"

so everything is as expected here. So you will try to push your json to the api. First thing that api will do is it will try to decode this json into expected Haskell data type. This will immediatelly fail in this json decoding part as Word8 won't be able to decode "-1" and Aeson library will rise the exception. If the exception wasn't rased by underlying aeson lib, we would catch it and rise "Address pool gap should be in range [10..100]".

If we would like to rise only single exception for AddressPoolGap one could define custom FromJSON instance that works only for [10..100] (ie, [minBound..maxBound]) and not to use custom/generic aeson instance https://github.com/input-output-hk/cardano-wallet/blob/15dd91b05112c16be506129e5c00e88114f8bd16/src/Cardano/Wallet/Kernel/AddressPoolGap.hs#L33 . This would do the trick in our case:

instance FromJSON AddressPoolGap where
    parseJSON = parseJSON >=> \gap -> case mkAddressPoolGap gap of
        Left err -> fail $ toString $ sformat build $ bprint
        ("Address pool gap should be in range ["%int%".."%int%"].")
        (minBound @AddressPoolGap)
        (maxBound @AddressPoolGap)
        Right x  -> pure x

or something along these lines. Anyway if you think this would be nice to have please open a new ticket and we will discuss it with rest of the team. For now I don't think this is necessary nor urgent to have

[akegalj]

hm - these types of error checks will gratelly depend on aeson version (msg might be modified in aeson) and on naming of the module where the function is located (if someone changes name of the module or name of the function itself msg will change)

I guess its ok as a temp solution. What do you think @denisshevchenko @uroboros ?

[denisshevchenko]

Agree, phrase in expectJSONError is too long and specific.

[piotr-iohk]

As for the error messages Address pool gap should be in range [10..100] and Word8 is either floating or will cause over or underflow I'd be for having one consistent message and obviously the first one is more user friendly and informative. I'll create a ticket. This is actually observed for addressPoolGap and index fields for certain values outside their ranges...

depend on ... naming of the module where the function is located (if someone changes name of the module or name of the function itself msg will change)

makes sense, I'll shorten the messages. 👍

[akegalj]

I don't think this is right error - right @denisshevchenko @uroboros ?

I believe we are missing case such as <|> try' @UpdateEosWalletError updateEosWalletError at https://github.com/input-output-hk/cardano-wallet/blob/develop/src/Cardano/Wallet/API/V1/ReifyWalletError.hs#L47

[akegalj]

fixed in my local branch - will push after we merge #337 (which fixes some failures) and test the patch

[piotr-iohk]

@akegalj I have created a ticket #342 and marked test EOSWALLETS_LIST_01, EOSWALLETS_LIST_02 - cannot get FO wallets with EOS endpoint with pendingWith. From my side it is ready to be merged.