[CBR-349] Pave the way for redemption #3373

edsko · 2018-08-08T12:27:10Z

This cleans up some of the code a bit, stubs out some handlers for the
redemption end point.

As agreed with squad 1, this also moves the redemption endpoint to be part of
transactions endpoint rather than the accounts endpoint. This makes more sense,
since redemption creates a new transaction (otherwise it was the odd one out on
the accounts endpoint, the only one requiring additional parameters in the
legacy implementation, or requiring the active wallet in the new
implementation).

Description

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

Linked issue

Type of change

🐞 Bug fix (non-breaking change which fixes an issue)
🛠 New feature (non-breaking change which adds functionality)
⚠️ Breaking change (fix or feature that would cause existing functionality to change)
🏭 Refactoring that does not change existing functionality but does improve things like code readability, structure etc
🔨 New or improved tests for existing code
⛑ git-flow chore (backport, hotfix, etc)

Developer checklist

I have read the style guide document, and my code follows the code style of this project.
If my code deals with exceptions, it follows the guidelines.
I have updated any documentation accordingly, if needed. Documentation changes can be reflected in opening a PR on cardanodocs.com, amending the inline Haddock comments, any relevant README file or one of the document listed in the docs directory.

Testing checklist

I have added tests to cover my changes.
All new and existing tests passed.

QA Steps

Screenshots (if available)

adinapoli-iohk

LGTM, but we can simplify our handler signature 😉

adinapoli-iohk · 2018-08-08T12:34:24Z

wallet-new/src/Cardano/Wallet/API/V1/Handlers/Transactions.hs

+          -> Maybe AccountIndex
+          -> Redemption
+          -> Handler (WalletResponse Transaction)
+redeemAda layer mWId mAccIx redemption = do


We can do better here -- if you replace your API types from using QueryParam to CaptureWalletId and CaptureAccountId, we can avoid the Maybe altogether.

I didn't write the endpoint, just went with the endpoint as specified by @KtorZ . I can change it of course but that didn't feel like it was my decision to make :)

Ok, @adinapoli-iohk insists that CaptureWalletId and co are the way to go here so will change, despite it being inconsistent with the other endpoint :)

I wouldn't mind having this endpoint placed under the Transactions group built the following way:

/wallets/{walletId}/accounts/{accountIndex}/certificates

(basically, as it was before, under the accounts API, but here under the transaction API).

In essence, it's a transaction, but still a very specific one related to one wallet and one account of that particular wallet.

I hear you, but at the same time if we do down this line of reasoning we would have to move the entire transaction machinery under /accounts, because you never generate transactions out of thin air, they are always originated from a source wallet and account, so I'm not sure I buy into the dream here 😁

To me, we should try to follow the RESTful principles (which I know some times are more art than science) -- we are manipulating transaction resources at the end of the day, because it's what we create and list , so that seems to suggest we should stick with the /transactions hierarchy here.

Yeah, let's get back to the principles here. We are creating a transactions, right, so we ought to do a POST /transactions. But, it's actually a kind of special transaction, it's a certificate redemption. Thus, I see three options here:

We consider certificates as a resource on its own and create a new set of handlers under /certificates. However, their representation is strictly identical to transactions, they don't have any representation on their own. So this feels weird.

We consider certificates as a sub-resource of a Transaction which also feels weird. It's just a special kind of transactions. So having something like /transactions/certificates feels really RPC-like. Going this way, let's be honest and simply name this /redemption-transactions ... Meh.

A third option would be to simply extend the current POST /transactions endpoint to accept either a Payment | Redemption, picking the right handler depending on which inputs are received. Extending the Redemption type to:

data Redemption = Redemption { redemptionRedemptionCode :: ShieldedRedemptionCode , redemptionMnemonic :: Maybe RedemptionMnemonic , redemptionSpendingPassword :: SpendingPassword , redemptionTarget :: PaymentSource } deriving (Eq, Show, Generic)

Even though PaymentSource feels like a wrong name now 🙃 as it is more like a FullyQualifiedAccount but well, you know what we say about naming things ^^"

What's your opinion on that @adinapoli-iohk ?

adinapoli-iohk · 2018-08-08T12:37:00Z

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

    , getTransactionFee
         :: Payment -> Resp m EstimatedFees
+    , redeemAda
+         :: Maybe WalletId


Ditto here -- these can be now purged from the Maybe.

adinapoli-iohk · 2018-08-08T12:37:16Z

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

+      where
+        -- Read-only operations
+        ro :: (Kernel.DB -> x) -> n x
+        ro g = g <$> liftIO (Kernel.getWalletSnapshot w)


KtorZ

LGTM

KtorZ · 2018-08-08T13:54:46Z

May I ask you however not to merge anything before:

#3372

I am smelling a small conflict there. CI is green, we just need a final approval on the story before it gets in.

adinapoli-iohk · 2018-08-08T15:52:05Z

@edsko I know you are probably going to 🔪 me, but after a bit of rethinking with @KtorZ we reached the middle ground of keeping this endpoint as-it-is but to get rid of the explicit passing of the WalletId and AccountIndex as QueryParam, which is a bit odd, and instead passing them explicit in the request body (as in, in the input JSON).

Sorry, pal. 🙉 🙉

parsonsmatt · 2018-08-08T19:00:09Z

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

+                        :> CaptureWalletId
+                        :> CaptureAccountId
+                        :> ReqBody '[ValidJSON] Redemption
+                        :> Post '[ValidJSON] (WalletResponse Transaction)


This API structure is called like:

transactions/certificates/3/321234

This is a little odd an difficult to read. The previous URL structure looked like:

wallets/3/accounts/321234/certificates

which is clearer on what those numbered parameters actually mean.

I think the route structure should include the wallets/:walletId/accounts/:accountIndex/ "prefixes" or they should be included as part of the Redemption datatype. Folding them into the Redemption datatype would make for prettier routes.

LOL, Looks like y'all came to the same conclusion!

Then I guess it's probably our most acceptable solution :D

edsko · 2018-08-09T07:28:02Z

@edsko I know you are probably going to hocho me, but after a bit of rethinking with @KtorZ we reached the middle ground of keeping this endpoint as-it-is but to get rid of the explicit passing of the WalletId and AccountIndex as QueryParam, which is a bit odd, and instead passing them explicit in the request body (as in, in the input JSON).

@adinapoli-iohk @KtorZ so concretely, what is the proposal now? And do you want me to do that in this PR?

adinapoli-iohk · 2018-08-09T07:45:54Z

so concretely, what is the proposal now? And do you want me to do that in this PR?

If Matthias is happy with that, I would be more than happy to have you do the amending 😉

KtorZ · 2018-08-09T08:46:52Z

@edsko
The proposal is:

Cardano/Wallet/API/V1/Transactions.hs

    :<|> "transactions" :> "certificates"
                        :> Summary "Redeem a certificate"
                        :> ReqBody '[ValidJSON] Redemption
                        :> Post '[ValidJSON] (WalletResponse Transaction)

Cardano/Wallet/API/V1/Types.hs

data Redemption = Redemption
    { redemptionRedemptionCode   :: ShieldedRedemptionCode
    , redemptionMnemonic         :: Maybe RedemptionMnemonic
    , redemptionSpendingPassword :: SpendingPassword
    , redemptionWalletId         :: WalletId
    , redemptionAccountIndex     :: AccountIndex
    } deriving (Eq, Show, Generic)

If you could amend this PR with this, that'd be wonderful 🦋

edsko · 2018-08-09T09:01:52Z

@KtorZ @adinapoli-iohk ok, will amend accordingly, and also push a separate commit that refactors out some common patterns in our V1 wrapping code for the new wallet layer.

This cleans up some of the code a bit, stubs out some handlers for the redemption end point. As agreed with squad 1, this also moves the redemption endpoint to be part of transactions endpoint rather than the accounts endpoint. This makes more sense, since redemption creates a new transaction (otherwise it was the odd one out on the accounts endpoint, the only one requiring additional parameters in the legacy implementation, or requiring the active wallet in the new implementation). Also: * Add WalletId and AccountIndex to Redemption, as requested. * Factor out common functions, use ExceptT to avoid deeply nested pattern matching. * Remove some time limits where they were not necessary. * In `setupPayment`, return an address decoding error in the same way that we do this everywhere.

KtorZ · 2018-08-09T14:42:38Z

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

+    :<|> "transactions" :> "certificates"
+                        :> Summary "Redeem a certificate"
+                        :> ReqBody '[ValidJSON] Redemption
+                        :> Post '[ValidJSON] (WalletResponse Transaction)


KtorZ · 2018-08-09T14:42:51Z

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

+    , redemptionWalletId         :: WalletId
+    -- ^ Redeem to this wallet
+    , redemptionAccountIndex     :: AccountIndex
+    -- ^ Redeem to this account index in the wallet


edsko · 2018-08-10T05:34:53Z

Merging, AppVeyor timeout. Reviewed by two squad leads.

edsko requested review from KtorZ and adinapoli-iohk August 8, 2018 12:27

adinapoli-iohk suggested changes Aug 8, 2018

View reviewed changes

KtorZ approved these changes Aug 8, 2018

View reviewed changes

adinapoli-iohk approved these changes Aug 8, 2018

View reviewed changes

parsonsmatt suggested changes Aug 8, 2018

View reviewed changes

parsonsmatt approved these changes Aug 8, 2018

View reviewed changes

edsko force-pushed the refactor/cbr-342-redeem-ada-prep branch from 5158d0f to f3771f2 Compare August 9, 2018 13:35

edsko force-pushed the refactor/cbr-342-redeem-ada-prep branch from f3771f2 to 36a5df3 Compare August 9, 2018 14:31

KtorZ reviewed Aug 9, 2018

View reviewed changes

edsko merged commit 0351188 into develop Aug 10, 2018

edsko deleted the refactor/cbr-342-redeem-ada-prep branch August 10, 2018 05:35

[CBR-349] Pave the way for redemption #3373

[CBR-349] Pave the way for redemption #3373

Uh oh!

Conversation

edsko commented Aug 8, 2018

Description

Linked issue

Type of change

Developer checklist

Testing checklist

QA Steps

Screenshots (if available)

Uh oh!

adinapoli-iohk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KtorZ left a comment

Choose a reason for hiding this comment

Uh oh!

KtorZ commented Aug 8, 2018

Uh oh!

adinapoli-iohk commented Aug 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edsko commented Aug 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adinapoli-iohk commented Aug 9, 2018

Uh oh!

KtorZ commented Aug 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

edsko commented Aug 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edsko commented Aug 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

edsko commented Aug 9, 2018 •

edited

Loading

KtorZ commented Aug 9, 2018 •

edited

Loading