[CBR-401] For the validate-address API endpoint, add an IsOurs field … #3646

uroboros · 2018-09-21T14:12:34Z

…to clarify the ambiguity of an address not being recognised

Linked issue

https://iohk.myjetbrains.com/youtrack/issue/CBR-401

Type of change

🛠 New feature (non-breaking change which adds functionality)
🏭 Refactoring that does not change existing functionality but does improve things like code readability, structure etc

Testing checklist

All new and existing tests passed.

adinapoli-iohk

Good start @uroboros , thanks!

I have posted a bunch of comments which didn't occur to me when we first discussed the plan of action.

Furthermore, I would like to get @KtorZ 's opinion on this, as this touches the V1 API.

Last but not least, can I also ask you to modify the CHANGELOG.md to include this important API breaking change? I think this is exactly the kind of things we should track in the changelog, hehe.

Apart from that, looks good!

adinapoli-iohk · 2018-09-21T14:27:34Z

wallet-new/src/Cardano/Wallet/API/V1/Swagger.hs

  & paths %~ (DELETE, "/api/internal/reset-wallet-state") `setDescription` resetWalletStateDescription
-  & paths %~ (POST, "/api/v1/transactions/fees") `setDescription` estimateFeesDescription
+  & paths %~ (POST,   "/api/v1/transactions/fees")        `setDescription` estimateFeesDescription
+  & paths %~ (GET,    "/api/v1/addresses")                `setDescription` getAddressDescription


Uhm, thorny -- I guess it's a question more for @KtorZ : how resilient is our magic combinator when it comes to "ambiguous" endpoints? Here /api/v1/addresses might refer both to the endpoint "give me all the addresses" but also to /api/v1/addresses/{addressId} which means "give me just this single address".

I have the suspect the combinator will try to apply this description to both endpoint, which is obviously not correct. Am I right? 🤔

Here /api/v1/addresses might refer both to the endpoint "give me all the addresses" but also to /api/v1/addresses/{addressId} which means "give me just this single address"

Na. GET /api/v1/addresses unambiguously refers to the list addresses endpoint. The single get would be referred to as: GET /api/v1/addresses/{addressId} ! So no issue here, that's resilient 💪

(as shown in the doc example: https://github.com/input-output-hk/cardano-sl/blob/develop/wallet-new/src/Cardano/Wallet/API/V1/Swagger.hs#L157-L161 )

adinapoli-iohk · 2018-09-21T14:30:08Z

wallet-new/src/Cardano/Wallet/API/V1/Types.hs

+--   In other words, `AddressNotRecognised` makes an inconclusive statement about
+--   an address, whereas `AddressIsOurs` is unambiguous.
+data AddressIsOurs = AddressIsOurs
+                   | AddressNotRecognised


what about reflecting the comment into the types as well? Rather than AddressNotRecognised, should we have something like:

data AddressOwnership = AddressIsOurs | AddressAmbiguousOwnership

probably not the best name in town, but would reflect the fact that the ownership is ambiguous: we really don't know if it's ours or not. AddressNotRecognised suggest to me something a bit too strong.

adinapoli-iohk · 2018-09-21T14:30:40Z

wallet-new/src/Cardano/Wallet/API/V1/Types.hs

+
+instance ToJSON (V1 AddressIsOurs) where
+    toJSON (V1 AddressIsOurs)        = String "is-ours"
+    toJSON (V1 AddressNotRecognised) = String "not-recognised"


We have an (alas unwritten) conversion that we use snake case for enums, so it would be "is_ours", for example.

That can be done generically though with some helpers written by hand or taken from https://hackage.haskell.org/package/aeson-casing-0.1.0.5/docs/Data-Aeson-Casing.html

We probably already have a few stuff doing this.

adinapoli-iohk · 2018-09-21T14:31:17Z

wallet-new/src/Cardano/Wallet/API/V1/Types.hs

+instance FromJSON (V1 AddressIsOurs) where
+    parseJSON (String "is-ours")        = pure (V1 AddressIsOurs)
+    parseJSON (String "not-recognised") = pure (V1 AddressNotRecognised)
+    parseJSON x = typeMismatch "Not a valid AddressIsOurs" x


If you haven't done so (haven't reached to that point of the PR perhaps) don't forget to write a roundtrip Aeson test inside the MarshallingSpecs!

Same remarks as above, what about leveraging generics here?

adinapoli-iohk · 2018-09-21T14:31:31Z

wallet-new/src/Cardano/Wallet/API/V1/Types.hs

+    declareNamedSchema _ =
+        pure $ NamedSchema (Just "V1AddressIsOurs") $ mempty
+            & type_ .~ SwaggerString
+            & enum_ ?~ ["is-ours", "not-recognised"]


snake_case.

Same about generics? Note that for now, our only option is with generics-sop (debatable), applying the same transformation as in the JSON instance.

https://github.com/input-output-hk/cardano-sl/blob/develop/wallet-new/src/Cardano/Wallet/API/V1/Generic.hs#L30-L35

adinapoli-iohk · 2018-09-21T14:32:35Z

wallet-new/src/Cardano/Wallet/API/V1/Types.hs

@@ -1221,12 +1259,14 @@ instance ToSchema WalletAddress where
            & ("id"            --^ "Actual address.")
            & ("used"          --^ "True if this address has been used.")
            & ("changeAddress" --^ "True if this address stores change from a previous transaction.")
+            & ("isOurs"        --^ "'is-ours' if this address is recognised as ours, 'not-recognised' if it has not (yet) been encountered")


I was thinking (perhaps @KtorZ can give his own cents): when we use the is suffix usually that is used for booleans, but this one is not. Should we have rather a ownership field? I think it sounds much better.

Agreed. is or has prefix usually implies a boolean kind of value which is anyway a bad thing to have in an API 😛
I like ownership but I'd rather have the haskell data-type match as well and renamed to AddressOwnership if we go for this.

how about?
data AddressOwnership = AddressOwnershipOurs | AddressOwnershipInconclusive

and then on the API level: "ownership" -> "ours" or "inconclusive"

also happy with AddressOwnershipAmbiguous

KtorZ · 2018-09-21T14:56:25Z

wallet-new/src/Cardano/Wallet/API/V1/Swagger.hs

+To reflect this, the V1 endpoint does not fail when an address is not recognised
+and returns a new field 'is-ours' which indicates either that an address is ours,
+or that it is 'not-recognised' (which may mean 'not recognised yet').
+|]


KtorZ · 2018-09-24T08:14:29Z

@adinapoli-iohk

Okay, updated 👍
Please have another quick look at it :)

KtorZ · 2018-09-24T08:16:25Z

wallet-new/src/Cardano/Wallet/Util.hs

@@ -27,7 +27,7 @@ headToLower []     = Nothing
 headToLower (x:xs) = Just $ toLower x : xs

 stripFieldPrefix :: String -> String
-stripFieldPrefix = dropWhile (not . isUpper)
+stripFieldPrefix = dropWhile (not . isUpper) . drop 1


NOTE: I took some freedom in adjusting the way I think it should work. It was dead-code until now though, not used anywhere, so this change is free of any impact 👍

Was just going to ask, cheers for clarifying 😁

KtorZ · 2018-09-24T08:18:24Z

wallet-new/test/MarshallingSpec.hs

@@ -87,6 +87,7 @@ spec = parallel $ describe "Marshalling & Unmarshalling" $ do
        aesonRoundtripProp @SyncProgress Proxy
        aesonRoundtripProp @SyncThroughput Proxy
        aesonRoundtripProp @AccountIndex Proxy
+        aesonRoundtripProp @AddressOwnership Proxy


Now that I think about it, this shouldn't actually compile 🤔 ...

adinapoli-iohk

@KtorZ Thank you so much for taking this over from Ryan. Please don't hate me, but I have dropped a few comments 😅

adinapoli-iohk · 2018-09-24T08:16:34Z

wallet-new/src/Cardano/Wallet/API/V1/Types.hs

@@ -1221,12 +1262,14 @@ instance ToSchema WalletAddress where
            & ("id"            --^ "Actual address.")
            & ("used"          --^ "True if this address has been used.")
            & ("changeAddress" --^ "True if this address stores change from a previous transaction.")
+            & ("ownership"     --^ "'isOurs' if this address is recognised as ours, 'ambiguousOwnership' if the node doesn't have information to make a unambiguous statement.")


to make an unambiguous statement? 🤔

Oui 🇫🇷

adinapoli-iohk · 2018-09-24T08:17:20Z

wallet-new/src/Cardano/Wallet/Util.hs

@@ -27,7 +27,7 @@ headToLower []     = Nothing
 headToLower (x:xs) = Just $ toLower x : xs

 stripFieldPrefix :: String -> String
-stripFieldPrefix = dropWhile (not . isUpper)
+stripFieldPrefix = dropWhile (not . isUpper) . drop 1


Was just going to ask, cheers for clarifying 😁

adinapoli-iohk · 2018-09-24T08:18:46Z

wallet-new/src/Cardano/Wallet/WalletLayer/Kernel/Addresses.hs

-        -- it's not even ours. In both cases we return that it is "not used"
-        -- (here) and "not a change address" (here). In the future we may want
-        -- to extend this endpoint with an "is ours" field.
+        -- it's not even ours. In both cases we return 'V1.AddressNotRecognised'.


Ops, this comment should also be amended with the new type constructor.

Good catch 👍

adinapoli-iohk · 2018-09-24T08:23:23Z

wallet-new/src/Cardano/Wallet/WalletLayer/Kernel/Addresses.hs

@@ -51,7 +51,7 @@ createAddress wallet
    -- | Creates a new 'WalletAddress'. As this is a brand new, fresh Address,
    -- it's fine to have 'False' for both 'isUsed' and 'isChange'.
    mkAddress :: Address -> WalletAddress
-    mkAddress addr = WalletAddress (V1 addr) False False
+    mkAddress addr = WalletAddress (V1 addr) False False (V1 V1.AddressAmbiguousOwnership)


Interesting default, but I cannot help but ponder -- are we sure that's a sensible one?

What happens now if I have just created a new address via Deadalus. Now this is neither used nor change, but it is mine.
But if we base everything about the helper function addressOwnershipFromUsed that’s going to be a slipper slope I fear 😲

I was wondering whether mkAddress should explicitly take an ownership argument so that you are forced to reason about ownership at the call site.

Plus, we would remove addressOwnershipFromUsed as it's too easy perhaps to misuse it?

Totally agree.

adinapoli-iohk · 2018-09-24T08:24:10Z

wallet-new/test/unit/Test/Spec/Addresses.hs

@@ -534,6 +534,7 @@ spec = describe "Addresses" $ do
                            addrId            = V1.V1 randomAddr
                          , addrUsed          = False
                          , addrChangeAddress = False
+                          , addrIsOurs        = V1.V1 V1.AddressNotRecognised


How does this even compile? 😲

adinapoli-iohk

Great job, I like it @KtorZ 😉

If one member of your squad is out of work to do, a nice ticket might be to improve the tests for address ownership to actually test that if you pass an address from a different wallet, it comes across as inconclusive and if you pass an address of a different account .. it should tell you it's yours as well? 🤔

adinapoli-iohk · 2018-09-25T06:52:56Z

wallet-new/src/Cardano/Wallet/WalletLayer/Kernel/Conv.hs

-    addressMeta    = acc ^. HD.hdAccountState . HD.hdAccountStateCurrent . cpAddressMeta cardanoAddress
+    cardanoAddress   = hdAddress ^. HD.hdAddressAddress . fromDb
+    addressMeta      = acc ^. HD.hdAccountState . HD.hdAccountStateCurrent . cpAddressMeta cardanoAddress
+    addressOwnership =


Me and @KtorZ talked about this and we realised this is unnecessary complication: if we are converting from a kernel type (which remember, it's stored in our DB to begin with), surely this is our address, because otherwise we wouldn't be able to construct an HDAddress without having the derivation index of the parent account etc. This implies ownership, therefore here it's simply V1.AddressIsOurs.

It's a good idea to bang a comment in the code as well to justify our rationale.

Amended with what was discussed above 👍

…to clarify the ambiguity of an address not being recognised Co-authored-by: KtorZ

adinapoli-iohk

Solid! Thanks @KtorZ 😉

…e-address [CBR-401] For the validate-address API endpoint, add an IsOurs field …

…hk/feature/cbr-401-validate-address [CBR-401] For the validate-address API endpoint, add an IsOurs field …

uroboros requested a review from adinapoli-iohk September 21, 2018 14:12

adinapoli-iohk suggested changes Sep 21, 2018

View reviewed changes

uroboros requested a review from KtorZ September 21, 2018 14:40

KtorZ reviewed Sep 21, 2018

View reviewed changes

KtorZ force-pushed the feature/cbr-401-validate-address branch from 0c9994c to ae07766 Compare September 24, 2018 08:12

KtorZ reviewed Sep 24, 2018

View reviewed changes

KtorZ force-pushed the feature/cbr-401-validate-address branch from ae07766 to e492c45 Compare September 24, 2018 08:19

adinapoli-iohk suggested changes Sep 24, 2018

View reviewed changes

KtorZ force-pushed the feature/cbr-401-validate-address branch from e492c45 to d87f4f1 Compare September 24, 2018 09:05

adinapoli-iohk approved these changes Sep 24, 2018

View reviewed changes

adinapoli-iohk reviewed Sep 25, 2018

View reviewed changes

[CBR-401] For the validate-address API endpoint, add an IsOurs field …

43f4043

…to clarify the ambiguity of an address not being recognised Co-authored-by: KtorZ

KtorZ force-pushed the feature/cbr-401-validate-address branch from d87f4f1 to 43f4043 Compare September 25, 2018 07:00

adinapoli-iohk approved these changes Sep 25, 2018

View reviewed changes

adinapoli-iohk merged commit 2982a95 into develop Sep 25, 2018

adinapoli-iohk deleted the feature/cbr-401-validate-address branch September 25, 2018 09:55

KtorZ pushed a commit that referenced this pull request Nov 9, 2018

Merge pull request #3646 from input-output-hk/feature/cbr-401-validat…

b8b0d52

…e-address [CBR-401] For the validate-address API endpoint, add an IsOurs field …

[CBR-401] For the validate-address API endpoint, add an IsOurs field … #3646

[CBR-401] For the validate-address API endpoint, add an IsOurs field … #3646

Conversation

uroboros commented Sep 21, 2018

Linked issue

Type of change

Testing checklist

adinapoli-iohk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adinapoli-iohk Sep 21, 2018 • 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

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

KtorZ commented Sep 24, 2018

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

adinapoli-iohk 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

adinapoli-iohk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adinapoli-iohk left a comment

Choose a reason for hiding this comment

adinapoli-iohk Sep 21, 2018 •

edited

Loading