Skip to content
This repository has been archived by the owner on Aug 18, 2020. It is now read-only.

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

Merged
merged 1 commit into from
Sep 25, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion wallet-new/integration/Functions.hs
Original file line number Diff line number Diff line change
Expand Up @@ -560,7 +560,7 @@ runAction wc action = do

let changeAddress = toList (txOutputs newTx) \\ toList paymentDestinations
-- NOTE: instead of this manual conversion we could filter WalletAddress from getAddressIndex
pdToChangeAddress PaymentDistribution{..} = WalletAddress pdAddress True True
pdToChangeAddress PaymentDistribution{..} = WalletAddress pdAddress True True (V1 AddressIsOurs)
realChangeAddressId = map addrId addressesAfterTransaction \\ map addrId addressesBeforeTransaction
changeWalletAddresses = filter ((`elem` realChangeAddressId) . addrId) addressesAfterTransaction

Expand Down
7 changes: 6 additions & 1 deletion wallet-new/src/Cardano/Wallet/API/V1/Migration/Types.hs
Original file line number Diff line number Diff line change
Expand Up @@ -177,8 +177,13 @@ instance Migrate V0.CAddress V1.WalletAddress where
eitherMigrate V0.CAddress{..} = do
addrId <- eitherMigrate cadId
let addrUsed = cadIsUsed
let addrChangeAddress = cadIsChange
addrChangeAddress = cadIsChange
addrOwnership = addressOwnershipFromUsed addrUsed
return V1.WalletAddress{..}
where
addressOwnershipFromUsed :: Bool -> V1 V1.AddressOwnership
addressOwnershipFromUsed used =
V1 $ if used then V1.AddressIsOurs else V1.AddressAmbiguousOwnership

-- | Migrates to a V1 `SyncProgress` by computing the percentage as
-- coded here: https://github.com/input-output-hk/daedalus/blob/master/app/stores/NetworkStatusStore.js#L108
Expand Down
20 changes: 17 additions & 3 deletions wallet-new/src/Cardano/Wallet/API/V1/Swagger.hs
Original file line number Diff line number Diff line change
Expand Up @@ -1051,6 +1051,19 @@ the `spendingPassword` field to `null` as that won't influence the fee
calculation.
|]

getAddressDescription :: Text
getAddressDescription = [text|
The previous version of this endpoint failed with an HTTP error when the given
address was unknown to the wallet.

This was misleading since an address that is unknown to the wallet may still
belong to the wallet (since it could be part of a pending transaction in
another instance of the same wallet).

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').
|]
Copy link
Contributor

Choose a reason for hiding this comment

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

Noice 👍


--
-- The API
Expand Down Expand Up @@ -1083,7 +1096,8 @@ api (compileInfo, curSoftwareVersion) walletAPI mkDescription = toSwagger wallet
, deSoftwareVersion = fromString $ show curSoftwareVersion
}
& info.license ?~ ("MIT" & url ?~ URL "https://raw.githubusercontent.com/input-output-hk/cardano-sl/develop/lib/LICENSE")
& paths %~ (POST, "/api/internal/apply-update") `setDescription` applyUpdateDescription
& paths %~ (POST, "/api/internal/postpone-update") `setDescription` postponeUpdateDescription
& paths %~ (POST, "/api/internal/apply-update") `setDescription` applyUpdateDescription
& paths %~ (POST, "/api/internal/postpone-update") `setDescription` postponeUpdateDescription
& 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/{address}") `setDescription` getAddressDescription
40 changes: 39 additions & 1 deletion wallet-new/src/Cardano/Wallet/API/V1/Types.hs
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ module Cardano.Wallet.API.V1.Types (
, NewExternalWallet (..)
, WalletAndTxHistory (..)
-- * Addresses
, AddressOwnership (..)
, AddressIndex
, AddressValidity (..)
, AddressAsBase58
Expand Down Expand Up @@ -183,7 +184,7 @@ import Cardano.Wallet.API.V1.Swagger.Example (Example, example,
genExample)
import Cardano.Wallet.Orphans.Aeson ()
import Cardano.Wallet.Types.UtxoStatistics
import Cardano.Wallet.Util (showApiUtcTime)
import Cardano.Wallet.Util (mkJsonKey, showApiUtcTime)

import qualified Pos.Binary.Class as Bi
import qualified Pos.Chain.Txp as Txp
Expand Down Expand Up @@ -265,6 +266,11 @@ genericSchemaDroppingPrefix prfx extraDoc proxy = do
_ -> err


optsADTCamelCase :: A.Options
optsADTCamelCase =
defaultOptions { A.constructorTagModifier = mkJsonKey }


--
-- Versioning
--
Expand Down Expand Up @@ -1202,6 +1208,35 @@ instance BuildableSafeGen AddressValidity where
buildSafeGen _ AddressValidity{..} =
bprint ("{ valid="%build%" }") isValid

-- | An address is either recognised as "ours" or not. An address that is not
-- recognised may still be ours e.g. an address generated by another wallet instance
-- will not be considered "ours" until the relevant transaction is confirmed.
--
-- In other words, `AddressAmbiguousOwnership` makes an inconclusive statement about
-- an address, whereas `AddressOwnership` is unambiguous.
data AddressOwnership
= AddressIsOurs
| AddressAmbiguousOwnership
deriving (Show, Eq, Generic, Ord)

instance ToJSON (V1 AddressOwnership) where
toJSON = genericToJSON optsADTCamelCase . unV1

instance FromJSON (V1 AddressOwnership) where
parseJSON = fmap V1 . genericParseJSON optsADTCamelCase

instance ToSchema (V1 AddressOwnership) where
declareNamedSchema _ =
pure $ NamedSchema (Just "V1AddressOwnership") $ mempty
& type_ .~ SwaggerString
& enum_ ?~ ["isOurs", "ambiguousOwnership"]

instance Arbitrary (V1 AddressOwnership) where
arbitrary = fmap V1 $ oneof
[ pure AddressIsOurs
, pure AddressAmbiguousOwnership
]

--------------------------------------------------------------------------------
-- Accounts
--------------------------------------------------------------------------------
Expand All @@ -1211,6 +1246,7 @@ data WalletAddress = WalletAddress
{ addrId :: !(V1 Core.Address)
, addrUsed :: !Bool
, addrChangeAddress :: !Bool
, addrOwnership :: !(V1 AddressOwnership)
} deriving (Show, Eq, Generic, Ord)

deriveJSON Serokell.defaultOptions ''WalletAddress
Expand All @@ -1221,12 +1257,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.")
Copy link
Contributor

Choose a reason for hiding this comment

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

to make an unambiguous statement? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Oui 🇫🇷

)

instance Arbitrary WalletAddress where
arbitrary = WalletAddress <$> arbitrary
<*> arbitrary
<*> arbitrary
<*> arbitrary

newtype AccountIndex = AccountIndex { getAccIndex :: Word32 }
deriving (Show, Eq, Ord, Generic)
Expand Down
2 changes: 1 addition & 1 deletion wallet-new/src/Cardano/Wallet/Util.hs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Was just going to ask, cheers for clarifying 😁


mkJsonKey :: String -> String
mkJsonKey s = fromMaybe s . headToLower $ stripFieldPrefix s
Expand Down
7 changes: 3 additions & 4 deletions wallet-new/src/Cardano/Wallet/WalletLayer/Kernel/Addresses.hs
Original file line number Diff line number Diff line change
Expand Up @@ -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.AddressIsOurs)



Expand Down Expand Up @@ -179,11 +179,10 @@ validateAddress rawText db = runExcept $ do
-- If the address is unknown to the wallet, it's possible that it's ours
-- but not yet used (at least as far as we know, it may be pending in
-- another instance of the same wallet of course), or it may be that
-- 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.AddressAmbiguousOwnership'.
return V1.WalletAddress {
addrId = V1 cardanoAddress
, addrUsed = False
, addrChangeAddress = False
, addrOwnership = (V1 V1.AddressAmbiguousOwnership)
}
12 changes: 10 additions & 2 deletions wallet-new/src/Cardano/Wallet/WalletLayer/Kernel/Conv.hs
Original file line number Diff line number Diff line change
Expand Up @@ -193,9 +193,17 @@ toAddress acc hdAddress =
V1.WalletAddress (V1 cardanoAddress)
(addressMeta ^. addressMetaIsUsed)
(addressMeta ^. addressMetaIsChange)
(V1 addressOwnership)
where
cardanoAddress = hdAddress ^. HD.hdAddressAddress . fromDb
addressMeta = acc ^. HD.hdAccountState . HD.hdAccountStateCurrent . cpAddressMeta cardanoAddress
cardanoAddress = hdAddress ^. HD.hdAddressAddress . fromDb
addressMeta = acc ^. HD.hdAccountState . HD.hdAccountStateCurrent . cpAddressMeta cardanoAddress
-- NOTE
-- In this particular case, the address had to be known by us. As a matter
-- of fact, to construct a 'WalletAddress', we have to be aware of pieces
-- of informations we can only have if the address is ours (like the
-- parent's account derivation index required to build the 'HdAddress' in
-- a first place)!
addressOwnership = V1.AddressIsOurs

-- | Converts a Kernel 'HdAddress' into a Cardano 'Address'.
toCardanoAddress :: HD.HdAddress -> Address
Expand Down
1 change: 1 addition & 0 deletions wallet-new/test/MarshallingSpec.hs
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ spec = parallel $ describe "Marshalling & Unmarshalling" $ do
aesonRoundtripProp @SyncProgress Proxy
aesonRoundtripProp @SyncThroughput Proxy
aesonRoundtripProp @AccountIndex Proxy
aesonRoundtripProp @(V1 AddressOwnership) Proxy

-- HttpApiData roundtrips
httpApiDataRoundtripProp @AccountIndex Proxy
Expand Down
1 change: 1 addition & 0 deletions wallet-new/test/unit/Test/Spec/Addresses.hs
Original file line number Diff line number Diff line change
Expand Up @@ -534,6 +534,7 @@ spec = describe "Addresses" $ do
addrId = V1.V1 randomAddr
, addrUsed = False
, addrChangeAddress = False
, addrOwnership = V1.V1 V1.AddressAmbiguousOwnership
}
withAddressFixtures 1 $ \_ layer _ _ -> do
res <- WalletLayer.validateAddress layer (sformat build randomAddr)
Expand Down