Get tx by id #1795
Get tx by id #1795
Conversation
paweljakubas
force-pushed
the
paweljakubas/1789/get-tx-by-id
branch
from
56871c3 to
41ab6f3
Compare
|
bors try |
paweljakubas
added
the
ADDING FEATURE
tryBuild succeeded |
| [ expectResponseCode HTTP.status200 | ||
| , expectField (#direction . #getApiT) (`shouldBe` Outgoing) | ||
| , expectField (#status . #getApiT) (`shouldBe` Pending) | ||
| ] |
There was a problem hiding this comment.
Note that this is subject to race-condition, especially with a slot-length of 0.2s. There are good chances that the transaction is already inserted at this stage. I'd remove this assertion and the next one about the wallet balance (which is collateral to this test), and simply keep the last two assertions of this scenario behind an eventually.
These would be sufficient for this test to show that transactions can be obtained using their ID 👍
There was a problem hiding this comment.
agree here. this poses potential race condition - corrected in d898b0d
| @@ -839,7 +840,112 @@ spec = do | |||
| oJson2 <- expectValidJSON (Proxy @([ApiTransaction n])) o2 | |||
| length <$> [oJson1, oJson2] `shouldSatisfy` all (== 0) | |||
|
|
|||
| it "TRANS_DELETE_01 - Cannot forget pending transaction when not pending anymorevia CLI" $ \ctx -> do | |||
| it "TRANS_GET_01 - Can get Incoming and Outgoing transaction" $ \ctx -> do | |||
There was a problem hiding this comment.
Same comment than for the API scenario applies here too 👍
There was a problem hiding this comment.
done in d898b0d
lib/core/src/Cardano/Wallet.hs
Outdated
| withNoSuchWallet wid $ readCheckpoint pk | ||
| txs <- lift $ readTxHistory pk Descending wholeRange Nothing | ||
| case (filter txPresent txs) of | ||
| [] -> do |
There was a problem hiding this comment.
Arg! This defies a bit the purpose of such endpoint. Being able to fetch a single transaction should be quick and swift; here we are loading the entire transaction history in memory and filtering it by hand! For exchanges processing MANY transactions a day, that's not going to work (especially because they'll be the primary consumer of this endpoint).
This should be replaced by a single SQL query to retrieve a single transaction from the database 👍
paweljakubas
force-pushed
the
paweljakubas/1789/get-tx-by-id
branch
from
9622834 to
9f5eba3
Compare
| let err' = ErrNoSuchTransaction tid | ||
| throwE (ErrGetTransactionNoSuchTransaction err') | ||
| Right (Just tx) -> | ||
| pure tx |
paweljakubas
force-pushed
the
paweljakubas/1789/get-tx-by-id
branch
from
d898b0d to
01150fb
Compare
|
bors r+ |
tryBuild succeeded |
paweljakubas
force-pushed
the
paweljakubas/1789/get-tx-by-id
branch
from
01150fb to
e454ede
Compare
|
bors r+ |
1795: Get tx by id r=paweljakubas a=paweljakubas # Issue Number <!-- Put here a reference to the issue this PR relates to and which requirements it tackles --> #1789 # Overview <!-- Detail in a few bullet points the work accomplished in this PR --> - [x] I have added getTransaction endpoint, and all needed implementations, errors - [x] I have updated swagger - [x] I have added integration test - [x] I have added CLI support - [x] I have updated CLI unit tests - [x] I have added CLI integration tests - [x] I have added support in ByronTransaction - [x] I have added getTx to DB - [x] I have implemented getTx in Sqlite - [x] I have implemented getTx in MVar - [x] I added properties for getTx and improved on MVar impl as a result # Comments <!-- Additional comments or screenshots to attach if any --> <!-- Don't forget to: ✓ Self-review your changes to make sure nothing unexpected slipped through ✓ Assign yourself to the PR ✓ Assign one or several reviewer(s) ✓ Once created, link this PR to its corresponding ticket ✓ Assign the PR to a corresponding milestone ✓ Acknowledge any changes required to the Wiki --> Co-authored-by: Pawel Jakubas <[email protected]>
Build failed |
paweljakubas
force-pushed
the
paweljakubas/1789/get-tx-by-id
branch
from
e454ede to
8e071fc
Compare
|
bors r+ |
Build succeeded |
Issue Number
#1789
Overview
Comments