New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add optional address parameter to listTransaction #4004

Merged

paweljakubas merged 29 commits into master from paweljakubas/add-address-parameter

Jul 21, 2023

Contributor

paweljakubas commented Jun 26, 2023 •

edited

Loading

Add support for optional address in api
Handle inputs, outputs, collateral inputs and collateral output
tests for shelley style
tests for byron style
tests for shared style
swagger update

Comments

Issue Number

https://cardanofoundation.atlassian.net/browse/ADP-2817

paweljakubas self-assigned this

paweljakubas force-pushed the paweljakubas/add-address-parameter branch 3 times, most recently from 76d1535 to 012e873 Compare

July 6, 2023 13:11

paweljakubas requested a review from paolino

July 6, 2023 13:13

paolino reviewed

View reviewed changes

lib/wallet/src/Cardano/Wallet/DB/Pure/Implementation.hs Outdated Show resolved Hide resolved

lib/wallet/src/Cardano/Wallet/DB/Pure/Implementation.hs Outdated Show resolved Hide resolved

paweljakubas force-pushed the paweljakubas/add-address-parameter branch 2 times, most recently from 9db64c6 to b203b2c Compare

July 14, 2023 06:49

paolino self-requested a review

July 14, 2023 09:08

paweljakubas requested a review from HeinrichApfelmus

July 14, 2023 09:11

paweljakubas changed the title ~~Add address parameter to listTransaction~~ Add optional address parameter to listTransaction

HeinrichApfelmus reviewed

View reviewed changes

specifications/api/swagger.yaml Show resolved Hide resolved

HeinrichApfelmus reviewed

View reviewed changes

lib/wallet/api/http/Cardano/Wallet/Api.hs Outdated

@@ @@ -604,6 +604,7 @@ type ListTransactions n = "wallets" @@
                   :> QueryParam "end" Iso8601Time
                   :> QueryParam "order" (ApiT SortOrder)
                   :> QueryParam "max_count" ApiLimit
+                  :> QueryParam "addressId" (ApiAddressIdT n)

Contributor

HeinrichApfelmus Jul 14, 2023

Suggested change

      
                :> QueryParam "addressId" (ApiAddressIdT n)
          
                :> QueryParam "address" (ApiAddressIdT n)

?

Contributor Author

paweljakubas Jul 18, 2023

I agree here, but complied with what is in InspectAddress and PutByronAddress. Nevertheless, address seems less confusing than addressId. We might want to change the legacy "addressId" in the future.

Contributor

HeinrichApfelmus Jul 18, 2023

Independent of which name we prefer (I do prefer "address"), we have to be consistent with the OpenAPI specification, which had specified address, I believe:

x-parametersAddress: &parametersAddress
  in: query
  name: address

Whenever specification and code differ, that's a bug in the code. 🤓

HeinrichApfelmus mentioned this pull request

App retrieves transaction history #3291

Open

paweljakubas force-pushed the paweljakubas/add-address-parameter branch from b203b2c to fd2f3a0 Compare

July 18, 2023 10:12

paweljakubas requested a review from HeinrichApfelmus

July 18, 2023 10:13

paolino approved these changes

View reviewed changes

Collaborator

paolino left a comment

LGTM

specifications/api/swagger.yaml

+                in: query
+                name: address
+                description: |
+                  An optional address.

Collaborator

paolino Jul 11, 2023

Shouldn't we explain the usage here?

lib/wallet/src/Cardano/Wallet/DB/Pure/Implementation.hs Outdated Show resolved Hide resolved

HeinrichApfelmus approved these changes

View reviewed changes

Contributor

HeinrichApfelmus left a comment

Thank you! 😊

I have two requests:

Factor out listTransactionsFilteredByAddress to make the integration tests easier to read (less vertical space)
Check the status of the transaction ID for both wallets instead of comparing balances.

Other than that, I think that this is good to merge.

lib/wallet/integration/src/Test/Integration/Scenario/API/Shared/Transactions.hs

+                      sendAmtToAddr ctx wSrc wDest a1 2
+                      sendAmtToAddr ctx wSrc wDest a4 2
+                      sendAmtToAddr ctx wSrc wDest a5 2

Contributor

HeinrichApfelmus Jul 18, 2023

Suggested change

      
            let listTransactionsFilteredByAddress wallet =
          
                    Link.listTransactions' @'Shared wallet
          
                        Nothing Nothing Nothing Nothing Nothing

In order to make the integration tests easier to read, I favor factoring out this common usage pattern.

Contributor Author

paweljakubas Jul 21, 2023

good suggestion! Refactored this for all styles

lib/wallet/integration/src/Test/Integration/Scenario/API/Shared/Transactions.hs Outdated

Comment on lines 2635 to 2623

+                      let linkList0 = Link.listTransactions' @'Shared wDest
+                              Nothing
+                              Nothing
+                              Nothing
+                              Nothing
+                              Nothing
+                              (Just (apiAddress addr0))
+                      rl0 <- request @([ApiTransaction n]) ctx linkList0 Default Empty

Contributor

HeinrichApfelmus Jul 18, 2023

Suggested change

      
                    let linkList0 = Link.listTransactions' @'Shared wDest
          
                            Nothing
          
                            Nothing
          
                            Nothing
          
                            Nothing
          
                            Nothing
          
                            (Just (apiAddress addr0))
          
                    rl0 <- request @([ApiTransaction n]) ctx linkList0 Default Empty
          
                    let query0 = listTransactionsFilteredByAddress wDest $ Just (apiAddress addr0)
          
                    rl0 <- request @([ApiTransaction n]) ctx query0 Default Empty

Contributor Author

paweljakubas Jul 21, 2023

done

lib/wallet/integration/src/Test/Integration/Scenario/API/Shared/Transactions.hs

+                              Nothing
+                              Nothing
+                              (Just (apiAddress addr1))
+                      rl1 <- request @([ApiTransaction n]) ctx linkList1 Default Empty

Contributor

HeinrichApfelmus Jul 18, 2023

Suggested change

      
                    rl1 <- request @([ApiTransaction n]) ctx linkList1 Default Empty
          
                    rl1 <- request @([ApiTransaction n]) ctx query1 Default Empty

As above, using listTransactionsFilteredByAddress.

Contributor Author

paweljakubas Jul 21, 2023

done

lib/wallet/integration/src/Test/Integration/Scenario/API/Shared/Transactions.hs

+                              Nothing
+                              Nothing
+                              (Just (apiAddress addr2))
+                      rl2 <- request @([ApiTransaction n]) ctx linkList2 Default Empty

Contributor

HeinrichApfelmus Jul 18, 2023

Suggested change

      
                    rl2 <- request @([ApiTransaction n]) ctx linkList2 Default Empty
          
                    rl2 <- request @([ApiTransaction n]) ctx query2 Default Empty

As above with listTransactionsFilteredByAddress.

Contributor Author

paweljakubas Jul 21, 2023

done

lib/wallet/integration/src/Test/Integration/Scenario/API/Shared/Transactions.hs Outdated

+                      sendAmtToAddr ctx wSrc wDest a3 2
+                      eventually "There are exactly 8 transactions for wDest" $ do
+                          let linkList = Link.listTransactions' @'Shared wDest

Contributor

HeinrichApfelmus Jul 18, 2023

listTransactionsFilteredByAddress as above.

It may be worth putting this function listTransactionsFilteredByAddress into the where clause.

Contributor Author

paweljakubas Jul 21, 2023

listTransactionsFilteredByAddress now in where for shelley, shared and byron styles

specifications/api/swagger.yaml Show resolved Hide resolved

lib/wallet/integration/src/Test/Integration/Scenario/API/Shared/Transactions.hs Outdated

+                           , expectResponseCode HTTP.status202
+                           ]
+                       eventually "dest wallet balance is higher than before" $ do
+                           rGet <- request @ApiWallet ctx

Contributor

HeinrichApfelmus Jul 18, 2023

👍 This is necessary to make the test robust for execution on an actual Cardano network, where concurrency, consensus, and timing can and will affect the results — typically leading to "flaky" test failure.

However, instead of checking the balances, I prefer checking that for each wallet, the transaction (identified by its transaction ID) has status in_ledger — this is the correct way to ensure that a transaction has been accommodated in both source and destination wallet.

Contributor Author

paweljakubas Jul 21, 2023

very reasonable idea! Did it for all styles - byron, shared and shelley!

lib/wallet/api/http/Cardano/Wallet/Api.hs Outdated

@@ @@ -604,6 +604,7 @@ type ListTransactions n = "wallets" @@
                   :> QueryParam "end" Iso8601Time
                   :> QueryParam "order" (ApiT SortOrder)
                   :> QueryParam "max_count" ApiLimit
+                  :> QueryParam "addressId" (ApiAddressIdT n)

Contributor

HeinrichApfelmus Jul 18, 2023

Independent of which name we prefer (I do prefer "address"), we have to be consistent with the OpenAPI specification, which had specified address, I believe:

x-parametersAddress: &parametersAddress
  in: query
  name: address

Whenever specification and code differ, that's a bug in the code. 🤓

paweljakubas and others added 12 commits

July 21, 2023 08:42


          introduction of address query param to listTransactions

32fae57


          add optional address to db layer

f301227


          use optional address in listTransactions

45be005


          integration test 1

9cbfc5b


          integration test 2

c1837dc


          integration test 3

ed8b6e1


          integration test 5

cf99da4


          debug

9b021a2


          handle inputs

3b6a6df


          integration test 6

25462da


          add collateral support

a303dc3


          shared integration test 1

9d5c658

paweljakubas added 17 commits

July 21, 2023 08:43


          SHARED_TRANSACTIONS_LIST_05 works

4adfb87


          SHARED_TRANSACTIONS_LIST_06 works

e906e5a


          byron integration test 1

c20977e


          byron integration test 2

83d79d0


          byron integration test 3

4b8f4b0


          byron integration test 4

18694ab


          byron integration test 5

e7c6246


          byron integration test 6

4c75456


          update swagger

0d01c52


          hlint

d4a7326


          code review adjustment

6a7c8f5


          listTransactionsFilteredByAddress for shelley

4feea0c


          listTransactionsFilteredByAddress for shared

429d5bb


          listTransactionsFilteredByAddress for byron

7ca0f92


          rely on in_ledger detection rather than balance comparison for shared

ada92ba


          rely on in_ledger detection rather than balance comparison for shelley

a434553


          rely on in_ledger detection rather than balance comparison for byron

212d85d

paweljakubas force-pushed the paweljakubas/add-address-parameter branch from 79c6ec7 to 212d85d Compare

July 21, 2023 12:51

paweljakubas added this pull request to the merge queue

Merged via the queue into master with commit 1388223

2 checks passed

paweljakubas deleted the paweljakubas/add-address-parameter branch

July 21, 2023 14:07

WilliamKingNoel-Bot pushed a commit that referenced this pull request


          docs: Add optional address parameter to listTransaction (#4004) <!-- …

24c18c4

…Detail in a few bullet points the work accomplished in this PR. Before you submit, don't forget to: CODE-OF-CONDUCT.md CONTRIBUTING.md LICENSE MAINTAINERS.md README.md bors.toml cabal.project default.nix docker-compose.yml docs flake.lock flake.nix floskell.json fourmolu.yaml hie-direnv.yaml lib nix prototypes reports scripts shell.nix specifications test touch.me.CI weeder.dhall Make sure the GitHub PR fields are correct: ✓ Set a good Title for your PR. ✓ Assign yourself to the PR. ✓ Assign one or more reviewer(s). ✓ Link to a Jira issue, and/or other GitHub issues or PRs. ✓ In the PR description delete any empty sections and all text commented in  - [x] Add support for optional address in api - [x] Handle inputs, outputs, collateral inputs and collateral output - [x] tests for shelley style - [x] tests for byron style - [x] tests for shared style - [x] swagger update ### Comments  ### Issue Number https://cardanofoundation.atlassian.net/browse/ADP-2817  Source commit: 1388223

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet