Display serialised tx endpoint #2977

paweljakubas · 2021-10-15T19:09:38Z

The endpoint acts in the context of a given wallet and is able to detect the wallet's resources. In detail, the enpdoint detects:

id of tx
fee
inputs, belonging to other wallets and the wallets - in that case it lookups utxo and give amount and derivation path
outputs, belonging to other wallets or to the wallets - in that case it either see its output or change (here it looks ahead using gap) and enrich info by current amount and derivation path of address
withdrawals, belonging to other wallet or the wallet
metadata
script validity

Comments

Issue Number

Anviking · 2021-10-26T14:26:17Z

lib/core/src/Cardano/Wallet.hs

+                    [] -> (txin, Nothing) -- will probably use error here
+                    path':_ -> (txin, Just (txout, path'))
+
+extAddr


We could probably find a more descriptive name than extAddr. Seems similar to the existing normalizeDelegationAddress, but perhaps different enough not to combine.

But... Why are we modifying the address in the first place? 🤔 That seems very off to me. We should be looking up addresses based on their payment key hashes only, right?

ad 1. yes, normaliseDelegationAddress does almost the same as extAddr
ad 2. Why we needed? Because in the cbor there is delegation address as represented in the chain. and we store fingerprints really, so we need to adjust them in order to compare.

lib/core/src/Cardano/Wallet.hs

Anviking · 2021-10-26T15:09:33Z

lib/core/src/Cardano/Wallet.hs

+  where
+    db = ctx ^. dbLayer @IO @s @k
+    tryGetTxOutPath cp walletAddrs txin =
+        case UTxO.lookup txin (totalUTxO mempty cp) of


Isn't totalUTxO mempty cp the same thing as utxo cp? But seems good that we don't remove the inputs from the pending txs — I take the pending txs wouldn't be decodable/displayable then.

Also, not sure we need to nor if we should, but we could:

Use changeUTxO pending s <> utxo (but not excluding pending inputs), to be able to resolve chains of dependent txs (I don't think this would work currently)

Use LSQ to (potentially) lookup the txin in the node's full UTxO

I was thinking about pending txs. I would for now use totalUTxO mempty cp and think about pending tx in the next move

lib/core/src/Cardano/Wallet.hs

Anviking · 2021-10-29T16:09:40Z

@paweljakubas Sorry, but I didn't see the point of most of the complicated (and seemingly a bit out-of-place) logic in lookupTx{Ins,Outs} 😅 I tried replacing most of it with just isOurs in 32480e1, and tests seemed to pass, except for in one place where I believe the test expected the wrong value (c0c9b73).

Could you let me know if it looks good to you or whether I have misunderstood something?

paweljakubas · 2021-10-29T17:53:20Z

maybe it is ok to reuse isOurs but you erased the code that adds change addresses to "candidate" addresses. Why it is needed? Because this endpoint should work after construction, signing and discovery. And after construction we can omit these change addresses, right? so we need to sneak them (here assumption is that pool gap restriction is used)

paweljakubas · 2021-10-29T18:05:25Z

lib/core/src/Cardano/Wallet.hs

-            , ix )
-    let walletAddrs = map f $ knownAddresses (getState cp)
-
-    --We are analyzing the next gapPool change addresses


so we need this changeAddrs, otherwise source wallet will not see change address in outputs UNTIL the wallet discovers tx (so after submit). That is consequence of decoupling construction/signing/submitting and persisting pending things only during submitting

paweljakubas · 2021-10-29T18:16:38Z

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

@@ -479,7 +479,7 @@ spec = describe "NEW_SHELLEY_TRANSACTIONS" $ do
        let addrDest = (addrs !! addrIx) ^. #id
        let expectedTxOutTarget = WalletOutput $ ApiWalletOutput
                { address = addrDest
-                , amount = Quantity 0


no it should be 0. This is what is currently in this address. So after construction, source wallet does not see this as WalletOutput but ExternalOutput. Target wallet see this as WalletOuptut, and see amountIncoming=Quantity amt, but amount=Quantity 0 as there is nothing at THIS stage in this wallet. Right? It is going to be amount=amountIncoming after discovery. I did this on purpose to have a way to see whole cycle if this resource is ours :-)

see line 548 and

let expectedTxOutTarget' = WalletOutput $ ApiWalletOutput { address = addrDest , amount = Quantity amt -- now we have this , assets = ApiT TokenMap.empty , derivationPath = NE.fromList [ ApiT (DerivationIndex 2147485500) , ApiT (DerivationIndex 2147485463) , ApiT (DerivationIndex 2147483648) , ApiT (DerivationIndex 0) , ApiT (DerivationIndex $ fromIntegral addrIx) ] , amountIncoming = Quantity amt , assetsIncoming = ApiT TokenMap.empty }

this is after submitting and discovering

clean

post rebase adjustments

some cleaning

This commit does sacrifice the distinction between amount and amount_incoming. The next commit addresses that.

My reason being the combination of: 1. Unclear why we need it for this task / now. There might be interesting things to do here, but that we should be clear about what problem we're trying to solve, such that we can create a solution we're fully confident in. 2. Exposing balances of individual addresses feels at odds with the point of the wallet otherwise being maintaining a concept of balance for all addresses combined. 3. It seemed to make the implementation much more complicated

1. Check that the change output is recognized 2. Use `shouldContain` and `shouldNotContain` rather than `shouldSatisfy` and `elem` for nicer error messages.

Anviking

lgtm! Hoping we at some point later on can get CBOR <-> JSON roundtrip properties, which I believe would be the ultimate test that we display all information!

Anviking · 2021-11-02T12:10:12Z

bors r+

Anviking · 2021-11-02T12:13:55Z

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

+            , expectField #outputs (`shouldNotContain` [expectedTxOutTarget])
+
+            -- Check that the change output is there:
+            , expectField (#outputs) ((`shouldBe` 1) . length . filter isOutOurs)


I added a check for the change output in the last commit along with a slight tweak, using should{Not}Contain [txOut] rather than shouldSatisfy and elem, which should produce nicer error messages and be slightly simpler.

good change

paweljakubas · 2021-11-02T12:26:52Z

@Anviking thanks for the thorough review and commits! I will create following ticket in which we property test using genTx and expose mint/burn and other things in this endpoint

iohk-bors · 2021-11-02T13:15:30Z

Build succeeded:

paweljakubas self-assigned this Oct 15, 2021

paweljakubas force-pushed the paweljakubas/adp-974/display-serialised-tx-endpoint branch 4 times, most recently from 2a0666c to db7a720 Compare October 22, 2021 13:22

paweljakubas marked this pull request as ready for review October 25, 2021 19:03

paweljakubas requested review from rvl and Anviking October 25, 2021 19:04

Anviking reviewed Oct 26, 2021

View reviewed changes

paweljakubas force-pushed the paweljakubas/adp-974/display-serialised-tx-endpoint branch from 128ca64 to e8ee5bf Compare October 28, 2021 15:09

paweljakubas requested a review from Anviking October 28, 2021 15:13

paweljakubas commented Oct 29, 2021

View reviewed changes

paweljakubas added 15 commits November 1, 2021 15:09

add first version of endpoint in swagger

7bda41c

add basic types

9728b09

change inputs to be more general in ApiDecodedTransaction

c007ecc

clean

deal with colls, inps and outs

c88c524

deal with withdrawals

d6b999c

deal with validity interval

c9f4b34

add lookupTxIns in Cardano.Wallet

df4f127

use lookupTxIns for inps and collaterals in decodeTransaction

bd277de

improve handling inputs

2d46976

generalize withdrawals in swagger

ab51465

deal with withdrawals

fd34bde

first integration test for decode tx

16d8bac

rm unneeded errors from ErrBalanceTx

802639a

unpend single output tx test

ef01d0f

fix address comparing in lookupTxIns

06bc139

paweljakubas added 13 commits November 1, 2021 15:17

finish integration test for inputs

12b1125

amend ExternalInput definition - we do not know amount from cbor here

4a9b08c

generalize outputs in swagger

6b1dc15

add generalized output type

2d5d30e

first impl of txout enricher

6b895e7

add more tests for outputs of wallet in integration testing

ef329b1

post rebase adjustments

add change addr detection

c23c889

some cleaning

use pool gap to determine number of candidate change addresses

428e9ba

add integration test for metadata

92eb97c

add integration testing for withdrawals

815761a

adjust core unit tests

0ae8f4f

fixing schema/json things

5aa90dd

use normaliseDelegationAddress

1746cac

Anviking force-pushed the paweljakubas/adp-974/display-serialised-tx-endpoint branch from 32480e1 to 1746cac Compare November 1, 2021 14:22

Anviking mentioned this pull request Nov 1, 2021

Simplifications to decode tx endpoint #2995

Merged

2 tasks

Anviking added 3 commits November 2, 2021 11:39

Simplify lookupTx{Ins,Outs} using isOurs

124c0ab

This commit does sacrifice the distinction between amount and amount_incoming. The next commit addresses that.

Tweak integration expectations

f7977b8

1. Check that the change output is recognized 2. Use `shouldContain` and `shouldNotContain` rather than `shouldSatisfy` and `elem` for nicer error messages.

Anviking force-pushed the paweljakubas/adp-974/display-serialised-tx-endpoint branch from 2dd3ccd to f7977b8 Compare November 2, 2021 12:03

Anviking approved these changes Nov 2, 2021

View reviewed changes

Anviking reviewed Nov 2, 2021

View reviewed changes

iohk-bors bot merged commit c8cbdb8 into master Nov 2, 2021

iohk-bors bot deleted the paweljakubas/adp-974/display-serialised-tx-endpoint branch November 2, 2021 13:15

WilliamKingNoel-Bot pushed a commit that referenced this pull request Nov 2, 2021

docs: Merge #2977

5a1c521

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display serialised tx endpoint #2977

Display serialised tx endpoint #2977

paweljakubas commented Oct 15, 2021 •

edited

Loading

Anviking Oct 26, 2021

paweljakubas Oct 28, 2021

Anviking Oct 26, 2021 •

edited

Loading

paweljakubas Oct 28, 2021

Anviking commented Oct 29, 2021

paweljakubas commented Oct 29, 2021

paweljakubas Oct 29, 2021

paweljakubas Oct 29, 2021

paweljakubas Oct 29, 2021

paweljakubas Oct 29, 2021

Anviking left a comment

Anviking commented Nov 2, 2021

Anviking Nov 2, 2021

paweljakubas Nov 2, 2021

paweljakubas commented Nov 2, 2021 •

edited

Loading

iohk-bors bot commented Nov 2, 2021

Display serialised tx endpoint #2977

Display serialised tx endpoint #2977

Conversation

paweljakubas commented Oct 15, 2021 • edited Loading

Comments

Issue Number

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anviking Oct 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anviking commented Oct 29, 2021

paweljakubas commented Oct 29, 2021

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

Anviking left a comment

Choose a reason for hiding this comment

Anviking commented Nov 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paweljakubas commented Nov 2, 2021 • edited Loading

iohk-bors bot commented Nov 2, 2021

paweljakubas commented Oct 15, 2021 •

edited

Loading

Anviking Oct 26, 2021 •

edited

Loading

paweljakubas commented Nov 2, 2021 •

edited

Loading