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

Tx metadata API integration tests #2096

Merged

KtorZ merged 3 commits into master from rvl/2074/tx-metadata-tests

Sep 2, 2020

Contributor

rvl commented Sep 1, 2020 •

edited

Loading

Issue Number

ADP-307 / #2074 / #2073.

Overview

API test case for posting a transaction with metadata.
Test creating a transaction with invalid metadata.
Test creating a transaction that's too large due to metadata.
Test that the transaction fee estimate is higher when there's metadata in the transaction.

Comments

Based on PR Include transaction metadata in coin selection and fee calculations #2104 branch.

rvl added the ADDING FEATURE label

rvl added this to the (ADP-307) Transaction metadata milestone

rvl self-assigned this

rvl force-pushed the rvl/2074/tx-metadata-tests branch 2 times, most recently from cc515c5 to 57297c4 Compare

September 1, 2020 14:17

rvl added this to In Progress in Adrestia

rvl requested a review from piotr-iohk

September 1, 2020 14:20

rvl force-pushed the rvl/2074/tx-metadata-tests branch from 57297c4 to a4a6d6a Compare

September 1, 2020 14:21

KtorZ approved these changes

View reviewed changes

Member

KtorZ left a comment

LGTM

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

@@ @@ -229,6 +241,7 @@ spec = do @@
                               between (feeMin + amt, feeMax + amt)
                           , expectField (#direction . #getApiT) (`shouldBe` Outgoing)
                           , expectField (#status . #getApiT) (`shouldBe` Pending)
+                          , expectField (#metadata . #getApiTxMetadata) (`shouldBe` Nothing)

Member

KtorZ Sep 1, 2020

👍

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

+                              , expectListField 0
+                                  (#metadata . #getApiTxMetadata)
+                                  (`shouldBe` Just (ApiT expected))
+                              ]

Member

KtorZ Sep 1, 2020

👍

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


		basePayload <- mkTxPayload ctx wb amt fixturePassphrase

		let txMeta = Aeson.object ["1" .= T.replicate 65 "a"]

Member

KtorZ Sep 1, 2020

👍

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

+                      let txMeta = Aeson.object
+                              [ (toText @Int i, Aeson.String (T.replicate 64 "a"))
+                              | i <- [0..1023] ]
+                      let payload = addTxMetadata txMeta basePayload

Member

KtorZ Sep 1, 2020

👍

rvl force-pushed the rvl/2073/api-tx-metadata branch from 257ace1 to 937f1df Compare

September 2, 2020 05:03

rvl force-pushed the rvl/2074/tx-metadata-tests branch from a4a6d6a to 843fdd0 Compare

September 2, 2020 05:03

rvl force-pushed the rvl/2073/api-tx-metadata branch from 937f1df to 5b2559e Compare

September 2, 2020 05:46

rvl force-pushed the rvl/2074/tx-metadata-tests branch from 843fdd0 to d27f70e Compare

September 2, 2020 06:01

rvl mentioned this pull request

Tx metadata extracted from binary blocks #2077

Closed

piotr-iohk approved these changes

View reviewed changes

Contributor

piotr-iohk left a comment

lgtm.

There are few other cases come to my mind, pertaining to invalid metadata but maybe a bit more subtle:

top-level index > 2^64-1

{ 999999999999999999999999999999999999999999999999999999999999999999999999999999999: "ziemniak" }

hex that is not hex

{ 1: { "hex": "ąćó" } }

map that is not map

{ "1": [ [ "k1", "v1", "v2" ] ] }

I wonder would that produce invalid metadata error?

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

+                          (Link.createTransaction @'Shelley wa) Default payload
+                      expectResponseCode @IO HTTP.status400 r
+                      expectErrorMessage "fixme: api validation error message" r

Contributor

piotr-iohk Sep 2, 2020

This message is unhappy, it's crying "fixme" ;)

Contributor Author

rvl Sep 2, 2020

Yes, I needed to run the tests first to find out what error message to expect. 🙂
Fixed now.

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

+                          (Link.createTransaction @'Shelley wa) Default payload
+                      expectResponseCode @IO HTTP.status400 r
+                      expectErrorMessage "fixme: transaction rejected message" r

Contributor

piotr-iohk Sep 2, 2020

so does this one

Contributor Author

rvl Sep 2, 2020

See latest update of the branch.
Turns out the error message for oversized transactions is not very good.
I think we should fix the error message - but in another PR.

Member

KtorZ Sep 2, 2020

Too late. I've done it in this PR 😬

Contributor Author

rvl Sep 3, 2020

Thanks

rvl mentioned this pull request

Include transaction metadata in coin selection and fee calculations #2103

Merged

rvl force-pushed the rvl/2073/api-tx-metadata branch from 5b2559e to 039372f Compare

September 2, 2020 11:19

rvl force-pushed the rvl/2074/tx-metadata-tests branch from d27f70e to dabe9e2 Compare

September 2, 2020 11:19

rvl changed the base branch from rvl/2073/api-tx-metadata to rvl/2075/fee

September 2, 2020 11:26

rvl marked this pull request as ready for review

September 2, 2020 11:26

KtorZ force-pushed the rvl/2075/fee branch 2 times, most recently from c78df67 to b8ae9ca Compare

September 2, 2020 14:38

KtorZ force-pushed the rvl/2074/tx-metadata-tests branch 2 times, most recently from 0a7665a to 01be0de Compare

September 2, 2020 16:04

KtorZ changed the base branch from rvl/2075/fee to master

September 2, 2020 16:06

Member

KtorZ commented Sep 2, 2020

bors r+

Contributor

iohk-bors bot commented Sep 2, 2020

This PR was included in a batch with a merge conflict, it will be automatically retried

iohk-bors bot added a commit that referenced this pull request


          Merge #2096

0c34ac5

2096: Tx metadata API integration tests r=KtorZ a=rvl

### Issue Number

ADP-307 / #2074 / #2073.

### Overview

- API test case for posting a transaction with metadata.
- Test creating a transaction with invalid metadata.
- Test creating a transaction that's too large due to metadata.
- Test that the transaction fee estimate is higher when there's metadata in the transaction.

### Comments

- Based on PR #2104 branch.

Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>
Co-authored-by: KtorZ <matthias.benkort@gmail.com>

rvl added 2 commits

September 2, 2020 18:27


          Include transaction metadata in coin selection and fee calculations

c87e879

t# Author:    Rodney Lorrimar <rodney.lorrimar@iohk.io>


          Add tx metadata API integration tests

a59bbe3

Contributor

iohk-bors bot commented Sep 2, 2020

This PR was included in a batch with a merge conflict, it will be automatically retried

22 similar comments

Contributor

iohk-bors bot commented Sep 2, 2020

This PR was included in a batch with a merge conflict, it will be automatically retried

Contributor

iohk-bors bot commented Sep 2, 2020

This PR was included in a batch with a merge conflict, it will be automatically retried

Contributor

iohk-bors bot commented Sep 2, 2020

This PR was included in a batch with a merge conflict, it will be automatically retried

Contributor

iohk-bors bot commented Sep 2, 2020

This PR was included in a batch with a merge conflict, it will be automatically retried

Contributor

iohk-bors bot commented Sep 2, 2020

This PR was included in a batch with a merge conflict, it will be automatically retried

Contributor

iohk-bors bot commented Sep 2, 2020

This PR was included in a batch with a merge conflict, it will be automatically retried

Contributor

iohk-bors bot commented Sep 2, 2020

This PR was included in a batch with a merge conflict, it will be automatically retried

Contributor

iohk-bors bot commented Sep 2, 2020

This PR was included in a batch with a merge conflict, it will be automatically retried

Contributor

iohk-bors bot commented Sep 2, 2020

This PR was included in a batch with a merge conflict, it will be automatically retried

Contributor

iohk-bors bot commented Sep 2, 2020

This PR was included in a batch with a merge conflict, it will be automatically retried

Contributor

iohk-bors bot commented Sep 2, 2020

This PR was included in a batch with a merge conflict, it will be automatically retried

Contributor

iohk-bors bot commented Sep 2, 2020

This PR was included in a batch with a merge conflict, it will be automatically retried

Contributor

iohk-bors bot commented Sep 2, 2020

This PR was included in a batch with a merge conflict, it will be automatically retried

Contributor

iohk-bors bot commented Sep 2, 2020

This PR was included in a batch with a merge conflict, it will be automatically retried

Contributor

iohk-bors bot commented Sep 2, 2020

This PR was included in a batch with a merge conflict, it will be automatically retried

Contributor

iohk-bors bot commented Sep 2, 2020

This PR was included in a batch with a merge conflict, it will be automatically retried

Contributor

iohk-bors bot commented Sep 2, 2020

This PR was included in a batch with a merge conflict, it will be automatically retried

Contributor

iohk-bors bot commented Sep 2, 2020

This PR was included in a batch with a merge conflict, it will be automatically retried

Contributor

iohk-bors bot commented Sep 2, 2020

This PR was included in a batch with a merge conflict, it will be automatically retried

Contributor

iohk-bors bot commented Sep 2, 2020

This PR was included in a batch with a merge conflict, it will be automatically retried

Contributor

iohk-bors bot commented Sep 2, 2020

This PR was included in a batch with a merge conflict, it will be automatically retried

Contributor

iohk-bors bot commented Sep 2, 2020

This PR was included in a batch with a merge conflict, it will be automatically retried

Contributor Author

rvl commented Sep 2, 2020

bors r-

Contributor

iohk-bors bot commented Sep 2, 2020

Canceled

rvl mentioned this pull request

Tx metadata included in fee calculations #2075

Closed

Contributor Author

rvl commented Sep 3, 2020

@piotr-iohk Thanks for the suggestions about test coverage for invalid metadata cases. I added a note to the QA section of #2073 about this.

These cases are covered in cardano-node/cardano-api/test/Test/Cardano/Api/MetaData.hs by property tests. So we only need one invalid metadata test (I chose one with a string that was too long) to test the API error path.

rvl mentioned this pull request

Tx metadata JSON in API transaction list #2074

Closed

piotr-iohk moved this from In Progress to Closed in Adrestia

CharlesMorgan-IOHK removed this from Closed in Adrestia

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