Skip to content
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.

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

Enhance integration tests relating to transaction metadata and fees #2749

Merged

Conversation

jonathanknowles
Copy link
Member

@jonathanknowles jonathanknowles commented Jul 1, 2021

Issue Number

ADP-1009

Overview

This PR doesn't provide a solution for ADP-1009, but it does add integration test coverage relating to transaction metadata and fees.

In principle, the following two endpoints, if called on a wallet with the same starting UTxO and coin selection criteria, should return identical transaction fees (modulo random selection):

Modifications made by this PR

This PR:

  • Adjusts TRANSMETA_CREATE_01 so that it tests with a variety of different coin selection criteria:
    • no metadata;
    • simple textual metadata;
    • the set of metadata recorded in ADP-1005.
  • Adjusts TRANSMETA_CREATE_01 so that it first performs a dry-run coin selection with the selectCoins endpoint.
  • Adjusts TRANSMETA_CREATE_01 so that it cross-checks the fee obtained from calling selectCoins with the fee obtained from calling postTransaction, failing if the fees are not identical.

This PR provides evidence that:

  • that the selectCoins and postTransaction endpoints do generate selections with identical fees, provided the coin selection criteria are identical.
  • that we can indeed make a transaction that includes the metadata recorded in ADP-1005, and that the transaction is not rejected by the ledger for having a fee that is too low.

@jonathanknowles jonathanknowles self-assigned this Jul 1, 2021
@jonathanknowles jonathanknowles force-pushed the jonathanknowles/enhance-integration-tests-for-metadata-fees branch from 3bdf00b to f2928e0 Compare July 1, 2021 08:48
let apiCoinSelection =
getFromResponse Prelude.id coinSelectionResponse
let fee = computeApiCoinSelectionFee apiCoinSelection
fee `shouldBe` Coin expectedFee
Copy link
Contributor

@piotr-iohk piotr-iohk Jul 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We check here if expectedFee equals result of hand written computeApiCoinSelectionFee, but might it be that Ledger hadrware device calculates it wrong?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We check here if expectedFee equals result of hand written computeApiCoinSelectionFee, but might it be that Ledger hadrware device calculates it wrong?

Does the ledger hardware device perform its own fee computation, or adjust the ada quantities in any way?

I was under the impression that it just signs the the transaction provided to it.

@jonathanknowles jonathanknowles force-pushed the jonathanknowles/enhance-integration-tests-for-metadata-fees branch 2 times, most recently from 2160509 to a1eabdd Compare July 2, 2021 05:47
By doing this, we can confirm that the 'selectCoins' endpoint produces a
selection whose fee is identical to the selection  produced by the
'postTransaction' endpoint.
This allows us to compare the fees before and after adding metadata.
…allets`.

Creating coin selections (with and without metadata) is now covered in:

`Test.Integration.Scenario.API.Shelley.Transactions`.
@jonathanknowles jonathanknowles force-pushed the jonathanknowles/enhance-integration-tests-for-metadata-fees branch from a1eabdd to 6abdb1f Compare July 2, 2021 05:50
Copy link
Contributor

@piotr-iohk piotr-iohk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@piotr-iohk
Copy link
Contributor

It seems that the issue was fixed by adding additional metadata entry so proper fee can be calculated on coin selection -> https://input-output-rnd.slack.com/archives/C819S481Y/p1625242073175000?thread_ts=1623941347.111200&cid=C819S481Y

In any case these are very good additional tests!

@piotr-iohk
Copy link
Contributor

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jul 2, 2021

Build succeeded:

@iohk-bors iohk-bors bot merged commit 3b0363f into master Jul 2, 2021
@iohk-bors iohk-bors bot deleted the jonathanknowles/enhance-integration-tests-for-metadata-fees branch July 2, 2021 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants