First end-to-end transaction Integration test (with http-bridge) #281
Conversation
Seems like we are _sometimes_ off by one byte. I say sometimes because, the node will sometimes be fine with our calculation, and sometimes compute a size that is one byte bigger than what we compute. This is rather weird and to be honest, we don't have time anymore to look deeper into this. Going for the simple approach here
| "Err submitting transaction: protocol failure: " | ||
| <> pretty err | ||
| } | ||
| ErrSubmitTxNoSuchWallet _ -> err404 |
There was a problem hiding this comment.
NOTE: Ideally, we should do this for pretty much all errors as well as making sure we add more context to our errors!
There was a problem hiding this comment.
This is good to have.
For an API consumer it is nicer to have error messages as JSON responses (e.g. { "status": "error", "message": "Submiting transaction: protocol failure" }). Then the API consumer can always parse all responses as JSON, rather than worrying about whether it will be text/plain or application/json.
There was a problem hiding this comment.
Indeed. Ideally, something more like { "code": <an_error_code>, "message": <description / message> }, the status is redundant with the HTTP status. However, having a clear discriminant is helpful for clients like Daedalus (instead of having to grep on the message).
| verify r' | ||
| [ expectFieldEqual balanceTotal oneMillionAda | ||
| , expectFieldEqual balanceAvailable oneMillionAda | ||
| ] | ||
|
|
||
| it "Single Output Transaction" $ \ctx -> do | ||
| (wa, wb) <- (,) <$> fixtureWallet ctx <*> fixtureWallet ctx |
There was a problem hiding this comment.
Why not write this over two lines?
There was a problem hiding this comment.
Hmmm.. Probably because conceptually, I still prefer using a monadic approach when the ordering of events is relevant. Here, using applicative makes it clear that these two actions could happen in any order, they don't depend on each other.
There was a problem hiding this comment.
Although I am fine with either ways, the do-notation is perhaps more readable.
| (wa, wb) <- (,) <$> fixtureWallet ctx <*> fixtureWallet ctx | ||
| (_, addrs) <- | ||
| unsafeRequest @[ApiAddress] ctx ("GET", getAddresses wb) Empty | ||
| let destination = (addrs !! 1) ^. #id |
There was a problem hiding this comment.
Why is this the second address from the wallet?
There was a problem hiding this comment.
The first one is already used to inject the initial funds from the faucet. So, it could be any addresses between the 1st and the 21th (with a default gap)
| verify r' | ||
| [ expectSuccess | ||
| , expectEventually ctx balanceAvailable (oneMillionAda + 1) | ||
| ] |
There was a problem hiding this comment.
Is it worth adding another expectation that the tx in wallet A is eventually confirmed?
There was a problem hiding this comment.
It would be! Once we have this feature 😬 ... We can't list or fetch transactions at the moment.
| "Err submitting transaction: protocol failure: " | ||
| <> pretty err | ||
| } | ||
| ErrSubmitTxNoSuchWallet _ -> err404 |
There was a problem hiding this comment.
This is good to have.
For an API consumer it is nicer to have error messages as JSON responses (e.g. { "status": "error", "message": "Submiting transaction: protocol failure" }). Then the API consumer can always parse all responses as JSON, rather than worrying about whether it will be text/plain or application/json.
Issue Number
Overview
Comments
I slightly adjusted the tx size estimator... Seems like we are sometimes off by one byte. I say sometimes because, the node will sometimes be fine with our calculation, and sometimes compute a size that is one byte bigger than what we compute. This is rather weird as I would expect the calculation of a transaction size to be 100% deterministic but cardano-sl is full of surprises. T be honest, we don't have time anymore to look deeper into this. Going for the simple approach here and added
+1to our estimate :/