Add forget pending transaction endpoint (Part1) #845

paweljakubas · 2019-10-16T13:04:07Z

Issue Number

Overview

I have added forget pending transaction endpoint, errors and types
I have updated swagger accordingly

Comments

KtorZ · 2019-10-16T13:14:06Z

lib/core/src/Cardano/Wallet.hs

+data ErrForgetPendingTx
+    = ErrForgetPendingTxNoSuchWallet ErrNoSuchWallet
+    | ErrForgetPendingTxNoSuchTransaction ErrNoSuchTransaction
+    | ErrForgetPendingTxTransactionIsNotPending (Hash "Tx")


Ah. Good point 😅

KtorZ · 2019-10-16T13:14:37Z

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

@@ -611,6 +614,23 @@ postExternalTransaction ctx (PostExternalTransactionData load) = do
    tx <- liftHandler $ W.submitExternalTx @ctx @t @k ctx load
    return $ ApiTxId (ApiT (txId @t tx))

+deleteTransaction
+    :: forall ctx s t k.
+        ( s ~ SeqState t


I think s ~ SeqState t is superfluous here.

indeed, fixed

KtorZ · 2019-10-16T13:15:19Z

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

+        ErrForgetPendingTxTransactionIsNotPending tid ->
+            apiError err404 TransactionNotPending $ mconcat
+                [ "The transaction with id : ", pretty tid,
+                  "cannot be forgotten as it is not pending anymore."


we'll need an extra blank space before "cannot" ☝️

KtorZ · 2019-10-16T13:16:03Z

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

+        ErrForgetPendingTxNoSuchTransaction e -> handler e
+        ErrForgetPendingTxTransactionIsNotPending tid ->
+            apiError err404 TransactionNotPending $ mconcat
+                [ "The transaction with id : ", pretty tid,


pretty will show a truncated transaction id, in the API response, we probably want to show the full id so I'd advise for toText instead.

good point - fixed

KtorZ · 2019-10-16T13:17:51Z

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

+instance FromText ApiTxId where
+    fromText txt = case fromText txt of
+        Left err -> Left $ TextDecodingError $ show err
+        Right tid -> Right $ ApiTxId $ ApiT tid


I think the FromText instance is confusing here 🤔 (especially because we have to still do some conversion on top in parseUrlPiece for the error).
I'd suggest to inline this with the definition in FromHttpApiData ApiTxId and return the right error type directly.

KtorZ · 2019-10-16T14:08:11Z

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

+      where
+          fromText' txt = case fromText txt of
+              Left err -> Left $ TextDecodingError $ show err
+              Right tid -> Right $ ApiTxId $ ApiT tid


Suggestion:

instance FromHttpApiData ApiTxId where parseUrlPiece txt = case fromText txt of Left (TextDecodingError err) -> Left $ T.pack err Right tid -> Right $ ApiTxId $ ApiT tid

jonathanknowles

Looks good!

However, I have a question regarding naming. There seems to be some inconsistency:

The endpoint operation name is called deleteTransaction (delete)
The response type is called responsesDeleteTransaction (delete)
The endpoint description is "forget pending transaction" (forget)
The wallet layer function is called forgetPendingTx (forget)
The associated error functions are called ErrForgetPendingTx... (forget)

For consistency, is there any way we can pick one of these (either "delete" or "forget") and use that consistently throughout?

I think it would make the code easier to follow. What do you think?

paweljakubas · 2019-10-16T14:26:29Z

@jonathanknowles I was thinking about it. So in order to follow REST API I would stick to DELETE, but underneath, like in Cardano.Wallet would use forget as we did for other endpoints - see postTransaction vs submitTransaction. When it comes to other naming I tried to follow already established convention (errors, etc)

KtorZ · 2019-10-16T14:33:21Z

I second @paweljakubas on this. delete makes sense at the API-level. Actually, we should only find names as get, put, patch, delete, list in the API-level. Yet, the wallet layer can (ought to) be more expressive and therefore, have functions with a clearer semantic when possible.

jonathanknowles · 2019-10-16T14:55:03Z

@KtorZ wrote:

Actually, we should only find names as get, put, patch, delete, list in the API-level

Just to clarify, what is the motivation behind this? Is it to restrict ourselves to just the verbs that are HTTP verbs?

We already seem to violate this principle in a few places. Consider the following API-level names:

listWallets: should this not be called getWallets?
listTransactions: should this not be called getTransactions?
migrateByronWallet: what should this be called, if not migrate?

…e the stuff through codebase

simplifying FromHttpApiData

paweljakubas · 2019-10-16T15:16:50Z

rebased and resolved conflicts

KtorZ · 2019-10-16T16:01:43Z

@jonathanknowles

list is a commonly used / accepted verb in the CRUD terminology. You sometimes see people even talking about CRUDL. Mapping that to the rest terminology, there are two options:

use list for collections
use of the plural form for collections, and singular form for singletons:
- getResource ==> GET /resources/{id} (or alternatively, getResourceById)
- getResources ==> GET /resources

I am fine with both so long as naming is consistent although I find the former more readable (use of list).

For migrateByronWallet, something like postByronWalletMigration will be more consistent with the rest. But as I said "we should only find names", I didn't say it was the case.

Having said that, we are really neat-picking here.

KtorZ · 2019-10-16T16:34:29Z

bors r+

iohk-bors · 2019-10-16T16:34:32Z

👎 Rejected by too few approved reviews

KtorZ · 2019-10-16T16:34:58Z

bors r+

834: DaedalusIPC: Fix isWindows function r=KtorZ a=rvl # Issue Number None. # Overview Fixes a typo in the OS detection code for NodeIPC. # Comments I again looked at the possibility of importing the cardano-shell package for this functionality. But it is pulling in more dependencies. Also the `daedalusIPC` function there had the logging replaced with `putTextLn`. Branch is [rvl/daedalus-ipc-cardano-shell](rvl/daedalus-ipc-fix...rvl/daedalus-ipc-cardano-shell) for reference though. 840: Make Jörmungandr NetworkLayer use raw blocks r=KtorZ a=Anviking # Issue Number #711 # Overview  - [x] I changed the constructors in `Cardano.Wallet.Jormungandr.Network` to create NetworkLayers with raw jörmungadnr-specific blocks, and later used `toWLBlock <$> nl`. This way we can later use `toSPBlock <$> nl` for stake-pools. # Comments   845: Add forget pending transaction endpoint (Part1) r=KtorZ a=paweljakubas # Issue Number  #836 # Overview  - [x] I have added forget pending transaction endpoint, errors and types - [x] I have updated swagger accordingly # Comments   Co-authored-by: Rodney Lorrimar <[email protected]> Co-authored-by: Johannes Lund <[email protected]> Co-authored-by: Pawel Jakubas <[email protected]>

iohk-bors · 2019-10-16T18:02:18Z

Build succeeded

paweljakubas mentioned this pull request Oct 16, 2019

Forgetting pending transactions #836

Closed

KtorZ reviewed Oct 16, 2019

View reviewed changes

KtorZ assigned paweljakubas Oct 16, 2019

paweljakubas requested a review from KtorZ October 16, 2019 13:58

KtorZ reviewed Oct 16, 2019

View reviewed changes

jonathanknowles self-requested a review October 16, 2019 14:08

jonathanknowles reviewed Oct 16, 2019

View reviewed changes

paweljakubas force-pushed the paweljakubas/836/forget-pending-tx branch from b80eab9 to e28d965 Compare October 16, 2019 14:39

paweljakubas requested a review from KtorZ October 16, 2019 14:48

paweljakubas added 3 commits October 16, 2019 17:05

add forget pending transaction endpoint, error and types and propagat…

ba29b5b

…e the stuff through codebase

update swagger

be53d9e

review changes

0ab2687

simplifying FromHttpApiData

paweljakubas force-pushed the paweljakubas/836/forget-pending-tx branch from e28d965 to c65982c Compare October 16, 2019 15:16

rebase fix

c65982c

KtorZ approved these changes Oct 16, 2019

View reviewed changes

iohk-bors bot merged commit c65982c into master Oct 16, 2019

KtorZ added this to the Forget Pending Transaction milestone Oct 17, 2019

KtorZ deleted the paweljakubas/836/forget-pending-tx branch October 18, 2019 13:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add forget pending transaction endpoint (Part1) #845

Add forget pending transaction endpoint (Part1) #845

paweljakubas commented Oct 16, 2019 •

edited

Loading

KtorZ Oct 16, 2019

KtorZ Oct 16, 2019

paweljakubas Oct 16, 2019

KtorZ Oct 16, 2019

KtorZ Oct 16, 2019

paweljakubas Oct 16, 2019

KtorZ Oct 16, 2019

paweljakubas Oct 16, 2019

KtorZ Oct 16, 2019

jonathanknowles left a comment •

edited

Loading

paweljakubas commented Oct 16, 2019 •

edited

Loading

KtorZ commented Oct 16, 2019

jonathanknowles commented Oct 16, 2019

paweljakubas commented Oct 16, 2019

KtorZ commented Oct 16, 2019 •

edited

Loading

KtorZ commented Oct 16, 2019

iohk-bors bot commented Oct 16, 2019

KtorZ commented Oct 16, 2019

iohk-bors bot commented Oct 16, 2019

Add forget pending transaction endpoint (Part1) #845

Add forget pending transaction endpoint (Part1) #845

Conversation

paweljakubas commented Oct 16, 2019 • edited Loading

Issue Number

Overview

Comments

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

jonathanknowles left a comment • edited Loading

Choose a reason for hiding this comment

paweljakubas commented Oct 16, 2019 • edited Loading

KtorZ commented Oct 16, 2019

jonathanknowles commented Oct 16, 2019

paweljakubas commented Oct 16, 2019

KtorZ commented Oct 16, 2019 • edited Loading

KtorZ commented Oct 16, 2019

iohk-bors bot commented Oct 16, 2019

KtorZ commented Oct 16, 2019

iohk-bors bot commented Oct 16, 2019

Build succeeded

paweljakubas commented Oct 16, 2019 •

edited

Loading

jonathanknowles left a comment •

edited

Loading

paweljakubas commented Oct 16, 2019 •

edited

Loading

KtorZ commented Oct 16, 2019 •

edited

Loading