Allow deleting expired transactions from the API #2262

rvl · 2020-10-22T12:40:43Z

Issue Number

ADP-93 / #1840

Overview

Using the existing transaction delete endpoint, also permit removing expired transactions.
Simplify DB model and state machine tests for this function
Integration test

Comments

Based on PR Add transaction TTL to payments API #2167 branch - merge that first.

piotr-iohk

LGTM.

I can confirm it works fine on testnet. Some minor comments also.

piotr-iohk · 2020-10-26T09:34:15Z

lib/core-integration/src/Test/Integration/Scenario/API/Shelley/Transactions.hs

@@ -2175,6 +2175,45 @@ spec = describe "SHELLEY_TRANSACTIONS" $ do
        txDeleteFromDifferentWalletTest emptyWallet "wallets"
        txDeleteFromDifferentWalletTest emptyRandomWallet "byron-wallets"

+    it "TRANS_TTL_DELETE_01 - Shelley: can remove expired tx" $ \ctx -> do
+        (wa, wb) <- (,) <$> fixtureWallet ctx <*> fixtureWallet ctx


wb could be an emptyWallet ctx I think.

Yep, fixed thanks.

piotr-iohk · 2020-10-26T09:37:55Z

lib/core-integration/src/Test/Integration/Scenario/API/Shelley/Transactions.hs

+        -- yes, gone
+        request @(ApiTransaction n) ctx linkDel Default Empty
+            >>= expectResponseCode @IO HTTP.status404
+


Would it make sense to add tests forgetting expired transactions that:

have metadata

are transactions with withdrawals?

I'm guessing it doesn't matter but perhaps it's good to have such cases in regression suite, just in case.
(As far as I see, playing manually, there's no problem forgetting such expired transactions)

I am confident that TTL is completely independent from these things.

piotr-iohk · 2020-10-26T09:39:49Z

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

            apiError err404 NoSuchTransaction $ mconcat
                [ "I couldn't find a transaction with the given id: "
                , toText tid
                ]
-        ErrRemovePendingTxTransactionNoMorePending tid ->
+        ErrRemoveTxAlreadyInLedger tid ->
            apiError err403 TransactionNotPending $ mconcat
                [ "The transaction with id: ", toText tid,
                  " cannot be forgotten as it is not pending anymore."


I think the message could be now:

Suggested change

" cannot be forgotten as it is not pending anymore."

" cannot be forgotten as it is already in ledger."

Fixed, thanks.

2167: Add transaction TTL to payments API r=rvl a=rvl ### Issue Number ADP-93 / #1840 ### Overview - [x] Add transaction TTL to swagger spec. User provides the value in seconds. - [x] Add transaction TTL to API types - [x] Add TTL slot calculation to API handler function - [x] Adjust mkStdTx to make ttl easier - it now takes the expiry slot directly. - [x] Integration tests ### Comments Next PRs - [x] Allow deleting expired transactions ⇒ #2262 - [x] Add CLI option `cardano-wallet transaction create [--ttl=SECONDS]` ⇒ #2267 - [ ] Perhaps clean up the large number of function arguments and return values in transaction layer functions. Co-authored-by: Rodney Lorrimar <[email protected]>

piotr-iohk · 2020-10-30T10:04:46Z

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

            apiError err404 NoSuchTransaction $ mconcat
                [ "I couldn't find a transaction with the given id: "
                , toText tid
                ]
-        ErrRemovePendingTxTransactionNoMorePending tid ->
+        ErrRemoveTxAlreadyInLedger tid ->
            apiError err403 TransactionNotPending $ mconcat


Perhaps also worth to change into TransactionNotInLedger?

Oops, yes fixed.

rvl · 2020-11-02T10:39:48Z

bors r+

2262: Allow deleting expired transactions from the API r=rvl a=rvl ### Issue Number ADP-93 / #1840 ### Overview - [x] Using the existing transaction delete endpoint, also permit removing expired transactions. - [x] Simplify DB model and state machine tests for this function - [x] Integration test ### Comments - Based on PR #2167 branch - merge that first. 2282: Increase bors timeout from 2h to 3h r=Anviking a=Anviking # Issue Number #2279 # Overview  - [x] Increase bors timeout from 2h to 3h # Comments It seems hydra is often slow to schedule/run the jobs, causing ≈18% of bors r+ to fail. It should be better to increase the timeout to 3h, than to have it fail. This doesn't affect the timeout of buildkite or hydra themselves.   Co-authored-by: Rodney Lorrimar <[email protected]> Co-authored-by: Johannes Lund <[email protected]>

iohk-bors · 2020-11-02T12:01:49Z

Build failed (retrying...):

buildkite/cardano-wallet

2262: Allow deleting expired transactions from the API r=rvl a=rvl ### Issue Number ADP-93 / #1840 ### Overview - [x] Using the existing transaction delete endpoint, also permit removing expired transactions. - [x] Simplify DB model and state machine tests for this function - [x] Integration test ### Comments - Based on PR #2167 branch - merge that first. Co-authored-by: Rodney Lorrimar <[email protected]>

rvl · 2020-11-02T12:23:00Z

I need to fix the integration tests.

bors r-

iohk-bors · 2020-11-02T12:23:02Z

Canceled.

rvl · 2020-11-04T06:34:42Z

bors r+

2262: Allow deleting expired transactions from the API r=rvl a=rvl ### Issue Number ADP-93 / #1840 ### Overview - [x] Using the existing transaction delete endpoint, also permit removing expired transactions. - [x] Simplify DB model and state machine tests for this function - [x] Integration test ### Comments - Based on PR #2167 branch - merge that first. Co-authored-by: Rodney Lorrimar <[email protected]>

iohk-bors · 2020-11-04T08:37:33Z

Build failed:

ci/hydra-build:required

Is a proper failure due to new integration test TRANS_TTL_DELETE_01.

piotr-iohk

LGTM

KtorZ

Nice.

KtorZ · 2020-11-04T15:54:09Z

lib/core/src/Cardano/Wallet/DB.hs

+data ErrRemoveTx
+    = ErrRemoveTxNoSuchWallet ErrNoSuchWallet
+    | ErrRemoveTxNoSuchTransaction (Hash "Tx")
+    | ErrRemoveTxAlreadyInLedger (Hash "Tx")


KtorZ · 2020-11-04T15:54:20Z

lib/core/src/Cardano/Wallet/DB/MVar.hs

-        , removePendingTx = \pk tid -> ExceptT $ do
-            alterDB errCannotRemovePendingTx db (mRemovePendingTx pk tid)
+        , removePendingOrExpiredTx = \pk tid -> ExceptT $ do
+            alterDB errCannotRemovePendingTx db (mRemovePendingOrExpiredTx pk tid)


KtorZ · 2020-11-04T15:55:17Z

lib/core/src/Cardano/Wallet/DB/Model.hs

+        Just txMeta | txMeta ^. #status == InLedger ->
+            ( Left (CantRemoveTxInLedger wid tid), wal )
+        Just _ ->
+            ( Right (), wal { txHistory = Map.delete tid (txHistory wal) } )


KtorZ · 2020-11-04T15:57:03Z

lib/core/src/Cardano/Wallet/DB/Sqlite.hs

+    selectFirst ((TxMetaStatus ==. W.InLedger):filt) [] >>= \case
+        Just _ -> pure 0  -- marked in ledger - refuse to delete
+        Nothing -> fromIntegral <$> deleteWhereCount
+            ((TxMetaStatus <-. [W.Pending, W.Expired]):filt)


For the same reason as TRANS_TTL_03.

rvl · 2020-11-06T12:57:41Z

bors r+

iohk-bors · 2020-11-06T14:42:12Z

Build succeeded:

2267: Add CLI option for transaction TTL r=rvl a=rvl ### Issue Number ADP-93 / #1840 ### Overview - [x] Hide shelley-specific CLI options in `cardano-wallet-jormungandr` (fixes #2169) - [x] Add option `cardano-wallet transaction create [--ttl=SECONDS]` - [ ] Update wiki page after merging. ### Comments - Based on PR #2262 branch - merge that first. 2297: Enable parallel integration tests in hydra r=Anviking a=Anviking # Issue Number ADP-466 # Overview  - [x] Pass in `-j 8` - [x] Only run CLI tests in parallel if `CI` env var isn't set - [x] Clean up some debug printing in tests # Comments   2302: Fix DB migration tests r=rvl a=piotr-iohk # Issue Number  # Overview - 098f6e5 Fix DB migration tests # Comments post release Co-authored-by: Rodney Lorrimar <[email protected]> Co-authored-by: Johannes Lund <[email protected]> Co-authored-by: Piotr Stachyra <[email protected]>

2267: Add CLI option for transaction TTL r=rvl a=rvl ### Issue Number ADP-93 / #1840 ### Overview - [x] Hide shelley-specific CLI options in `cardano-wallet-jormungandr` (fixes #2169) - [x] Add option `cardano-wallet transaction create [--ttl=SECONDS]` - [ ] Update wiki page after merging. ### Comments - Based on PR #2262 branch - merge that first. Co-authored-by: Rodney Lorrimar <[email protected]>

2267: Add CLI option for transaction TTL r=rvl a=rvl ### Issue Number ADP-93 / #1840 ### Overview - [x] Hide shelley-specific CLI options in `cardano-wallet-jormungandr` (fixes #2169) - [x] Add option `cardano-wallet transaction create [--ttl=SECONDS]` - [ ] Update wiki page after merging. ### Comments - Based on PR #2262 branch - merge that first. Co-authored-by: Rodney Lorrimar <[email protected]> Co-authored-by: Piotr Stachyra <[email protected]>

This reverts commit 970bebb, reversing changes made to eff3c57.

2267: Add CLI option for transaction TTL r=rvl a=rvl ### Issue Number ADP-93 / #1840 ### Overview - [x] Hide shelley-specific CLI options in `cardano-wallet-jormungandr` (fixes #2169) - [x] Add option `cardano-wallet transaction create [--ttl=SECONDS]` - [ ] Update wiki page after merging. ### Comments - Based on PR #2262 branch - merge that first. Co-authored-by: Rodney Lorrimar <[email protected]> Co-authored-by: Piotr Stachyra <[email protected]>

rvl mentioned this pull request Oct 22, 2020

Add transaction TTL to payments API #2167

Merged

8 tasks

rvl force-pushed the rvl/1840/delete-tx-expired branch from 67f25a9 to 1057537 Compare October 26, 2020 06:31

rvl self-assigned this Oct 26, 2020

rvl added the ADDING FEATURE Mark a PR as adding a new feature, for auto-generated CHANGELOG label Oct 26, 2020

rvl added this to the (ADP-93) Transaction scheduler milestone Oct 26, 2020

rvl marked this pull request as ready for review October 26, 2020 06:47

rvl force-pushed the rvl/1840/delete-tx-expired branch from fcefefb to d804856 Compare October 26, 2020 07:44

rvl mentioned this pull request Oct 26, 2020

Add CLI option for transaction TTL #2267

Merged

3 tasks

piotr-iohk approved these changes Oct 26, 2020

View reviewed changes

Base automatically changed from rvl/1840/tx-ttl-api to master October 29, 2020 17:55

piotr-iohk reviewed Oct 30, 2020

View reviewed changes

rvl force-pushed the rvl/1840/delete-tx-expired branch from 9580b98 to 737ffd3 Compare November 2, 2020 09:24

rvl mentioned this pull request Nov 2, 2020

Increase bors timeout from 2h to 4h #2282

Merged

1 task

rvl force-pushed the rvl/1840/delete-tx-expired branch 2 times, most recently from 866e53f to eb74e0e Compare November 3, 2020 15:18

rvl requested review from KtorZ and piotr-iohk November 4, 2020 07:26

piotr-iohk approved these changes Nov 4, 2020

View reviewed changes

KtorZ approved these changes Nov 4, 2020

View reviewed changes

rvl added 4 commits November 6, 2020 18:47

Also permit deleting expired transactions

4eceb7e

Simplify mRemovePendingTx model and state machine

594d7c3

Add integration test for deleting expired transactions

9cee2c6

Review rework

77ca6c8

rvl force-pushed the rvl/1840/delete-tx-expired branch from eb74e0e to f61495f Compare November 6, 2020 08:52

Mark test TRANS_TTL_DELETE_01 pending

d9da868

For the same reason as TRANS_TTL_03.

rvl force-pushed the rvl/1840/delete-tx-expired branch from f61495f to d9da868 Compare November 6, 2020 12:40

iohk-bors bot merged commit 970bebb into master Nov 6, 2020

iohk-bors bot deleted the rvl/1840/delete-tx-expired branch November 6, 2020 14:42

Anviking added a commit that referenced this pull request Nov 12, 2020

Revert "Merge #2262"

030c15f

This reverts commit 970bebb, reversing changes made to eff3c57.

Anviking mentioned this pull request Nov 13, 2020

TMP: Bisect property failure #2315

Closed

Anviking added a commit that referenced this pull request Nov 13, 2020

Revert "Merge #2262"

cd5ccc3

This reverts commit 970bebb, reversing changes made to eff3c57.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow deleting expired transactions from the API #2262

Allow deleting expired transactions from the API #2262

rvl commented Oct 22, 2020 •

edited

Loading

piotr-iohk left a comment

piotr-iohk Oct 26, 2020

rvl Oct 27, 2020

piotr-iohk Oct 26, 2020

rvl Oct 27, 2020

piotr-iohk Oct 26, 2020

rvl Oct 27, 2020

piotr-iohk Oct 30, 2020

rvl Nov 2, 2020

rvl commented Nov 2, 2020

iohk-bors bot commented Nov 2, 2020

rvl commented Nov 2, 2020

iohk-bors bot commented Nov 2, 2020

rvl commented Nov 4, 2020

iohk-bors bot commented Nov 4, 2020 •

edited by rvl

Loading

piotr-iohk left a comment

KtorZ left a comment

KtorZ Nov 4, 2020

KtorZ Nov 4, 2020

KtorZ Nov 4, 2020

KtorZ Nov 4, 2020

rvl commented Nov 6, 2020

iohk-bors bot commented Nov 6, 2020

	" cannot be forgotten as it is not pending anymore."
	" cannot be forgotten as it is already in ledger."

Allow deleting expired transactions from the API #2262

Allow deleting expired transactions from the API #2262

Conversation

rvl commented Oct 22, 2020 • edited Loading

Issue Number

Overview

Comments

piotr-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

rvl commented Nov 2, 2020

iohk-bors bot commented Nov 2, 2020

rvl commented Nov 2, 2020

iohk-bors bot commented Nov 2, 2020

rvl commented Nov 4, 2020

iohk-bors bot commented Nov 4, 2020 • edited by rvl Loading

piotr-iohk left a comment

Choose a reason for hiding this comment

KtorZ 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

rvl commented Nov 6, 2020

iohk-bors bot commented Nov 6, 2020

rvl commented Oct 22, 2020 •

edited

Loading

iohk-bors bot commented Nov 4, 2020 •

edited by rvl

Loading