Handle reclaims in get transaction

[paweljakubas]

• Update of ApiTransaction to include depositTaken and depositReturned
• adding of integration tests showing the case for joining/rejoining and quitting
• update core unit tests and swagger

Comments

Issue Number

adp-460
adp-1402

[Anviking]

lgtm

Also adding a returned deposit check for quitting in the old workflow would be nice

I.e. here lib/core-integration/src/Test/Integration/Scenario/API/Shelley/StakePools.hs:395

       -- Quit delegation altogether.
        rq <- quitStakePool @n ctx (src, fixturePassphrase)
        verify rq
            -- pending tx for quitting pool is initially outgoing
            -- because there is a fee for this tx
            [ expectResponseCode HTTP.status202
            , expectField (#status . #getApiT) (`shouldBe` Pending)
            , expectField (#direction . #getApiT) (`shouldBe` Incoming)
            ]

[Anviking]

This looked odd to me at first, but I guess it is correct, as the ins, outs and withdrawals include both ours and foreign.

[piotr-iohk]

@paweljakubas I see an issue:

1. When doing quit using Construct->sign->submit flow then:

• while tx is pending then GET /transactions/ID shows deposit_returned = 2A ✔️
• but when tx is in_ledger then GET /transactions/ID shows deposit_returned = 0 ❌

2. When doing old quit pool DELETE /stake-pools/*/wallets/{walletId} then:

• while tx is pending then GET /transactions/ID shows deposit_returned = 0 ❌
• and when tx is in_ledger then GET /transactions/ID shows deposit_returned = 0 ❌

I've added checks to illustrate this to repective tests:

1. TRANS_NEW_JOIN_01a
2. STAKE_POOLS_JOIN_01rewards

[paweljakubas]

@piotr-iohk @Anviking now all checks in tests added by Piotr passes. the problem was solved by:

• different formula for detecting reclaim- I made extensive comment in the code about rationale here
• now reclaim can only be for Incoming
• in old tx flow Incoming was when pending/inLedger
• in new tx flow Incoming was only for inLedger so I made sure it is also for Pending. In order to do it I needed to bolster submitTransaction, and its tx context.

[Anviking]

Everything TxMeta-related seems so complicated, and I don't grasp the full picture, but this change might interplay with the rollback logic

https://github.com/input-output-hk/cardano-wallet/blob/13a6c0b2c865092011ca0bc8c5ee2e7d7194365b/lib/core/src/Cardano/Wallet/DB/Sqlite.hs#L670-L673

It seems a pending Quit tx could be deleted on rollback. I'm not sure if it also stops the wallet from trying to re-submit it.

[paweljakubas]

I bet it is not going to destroy anything as for OLD tx workflow Incoming was for both Pending/InLedger. We had Outcoming -> Incoming for new tx workflow though. and now we have the transition/directions as for old tx workflow

[Anviking]

Fair, the old tx workflow should also be able to produce pending incoming txs through this logic.

I still suspect this could be a problem, but unrelated to this PR.

[piotr-iohk]

@paweljakubas I'm getting issues (arithmetic underflow in the log) when doing list transactions for my wallets on testnet on this branch:

$ curl -vX GET http://localhost:8090/v2/wallets/1ceb45b37a94c7022837b5ca14045f11a5927c65/transactions
Note: Unnecessary use of -X or --request, GET is already inferred.
*   Trying 127.0.0.1:8090...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 8090 (#0)
> GET /v2/wallets/1ceb45b37a94c7022837b5ca14045f11a5927c65/transactions HTTP/1.1
> Host: localhost:8090
> User-Agent: curl/7.68.0
> Accept: */*
> 
* Empty reply from server
* Connection #0 to host localhost left intact
curl: (52) Empty reply from server

in the wallet.log there is:

[RequestId 15] [GET] /v2/wallets/1ceb45b37a94c7022837b5ca14045f11a5927c65/transactions
arithmetic underflow

[paweljakubas]

I see this also in integration tests when running
$ stack test --ta '-m "TRANS_LIST"' cardano-wallet:integration

arithmetic underflow
    TRANS_LIST_03 - Can order results FAILED [1] (13428ms)

CLI Specifications, SHELLEY_CLI_TRANSACTIONS, TRANS_LIST_03 - Can order results

I bet I need to make sure not to substract bigger natural from smaller one in reclaimIfAny

[paweljakubas]

@piotr-iohk indeed, my hypothesis was right. I needed one more check in reclaimIfAny. Now the test passes. Could you kindly confirm?

[Anviking]

This logic is called from submitTransaction, where the user could submit any kind of (wallet-related) transaction.

Defining the direction this way might work for txs from constructTransaction, but with balanceTransaction it would quickly break.

What was the reason we can't use mkTxMeta instead of constructTxMeta? (which itself is duplicated with logic in Primitive.Model)

[paweljakubas]

well, we are not doing selection when submitting, we are decoding what is inside tx cbor to be sent. hence we do not use, mkTxMeta. But, we can resign from dir and compare amtInps and amtOuts to determine direction. Now it should survive balanceTransaction step (or at least we will not deteriorate situation in the context what was before this PR). I have refactored constructTxMeta and found a bug in submitTransaction. And simplified the things. And locally it is green
---> see the last commit

[paweljakubas]

bors r+

[iohk-bors]

Build failed:

• buildkite/cardano-wallet

#expected (hlint)

[paweljakubas]

bors r+

[iohk-bors]

Build failed:

• ci/hydra-build:required

  test/unit/Cardano/Wallet/Api/ServerSpec.hs:141:5: 
  1) Cardano.Wallet.Api.Server, API Server, handles bad host name
       uncaught exception: IOException of type UserError
       user error (Network.Socket.gai_strerror not supported: 11002)

  To rerun use: --match "/Cardano.Wallet.Api.Server/API Server/handles bad host name/"

Randomized with seed 406019484

#3121

[paweljakubas]

bors r+

[piotr-iohk]

adp-1402 situation seems to be fixed also. 🎉

It would be nice to have a property/unit test for that (integration or e2e is not really viable because most lakely it'd cause race conditions), but as far as I understand there's no way currently to have an automated test, so manual check has to be enough.

[iohk-bors]

Build failed:

• buildkite/cardano-wallet

Failures:
 
  src/Test/Integration/Scenario/CLI/Shelley/Transactions.hs:244:59: 
  1) CLI Specifications, SHELLEY_CLI_TRANSACTIONS, TRANS_CREATE_06 - Invalid amount, 1.5
       uncaught exception: IOException of type ResourceVanished
       fd:106: hFlush: resource vanished (Broken pipe)
 
  To rerun use: --match "/CLI Specifications/SHELLEY_CLI_TRANSACTIONS/TRANS_CREATE_06 - Invalid amount/1.5/"
 
Randomized with seed 2108903879
 
Finished in 960.1969 seconds, used 844.4028 seconds of CPU time
898 examples, 1 failure, 41 pending

#2855

[paweljakubas]

bors r+

[iohk-bors]

Timed out.

[paweljakubas]

bors retry

[iohk-bors]

Timed out.

[paweljakubas]

bors r+

[iohk-bors]

Canceled.