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

Hash check in transaction build tests #6023

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

palas
Copy link
Contributor

@palas palas commented Oct 24, 2024

DO NOT MERGE: Need to remove the SRP stanza

Description

This PR updates tests in cardano-testnet to work with the new check of anchor hashes added to transaction build (see IntersectMBO/cardano-cli#951).

Related PRs

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated. These may include:
    • golden tests
    • property tests
    • roundtrip tests
    • integration tests
      See Runnings tests for more details
  • Any changes are noted in the CHANGELOG.md for affected package
  • The version bounds in .cabal files are updated
  • CI passes. See note on CI. The following CI checks are required:
    • Code is linted with hlint. See .github/workflows/check-hlint.yml to get the hlint version
    • Code is formatted with stylish-haskell. See .github/workflows/stylish-haskell.yml to get the stylish-haskell version
    • Code builds on Linux, MacOS and Windows for ghc-8.10.7 and ghc-9.2.7
  • Self-reviewed the diff

Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

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

LGTM! A few suggestions.

-- | Adds environment variables to an 'ExecConfig' that may already
-- have some environment variables set. This is done by prepending the new
-- environment variables to the existing ones.
addEnvVarsToConfig :: H.ExecConfig -> [(String, String)] -> H.ExecConfig
Copy link
Contributor

Choose a reason for hiding this comment

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

We should modify mkExecConfig to accept a list of environment variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case, it is good to have it locally modified though, cause it is only relevant inside the serveFilesWhile

Copy link
Contributor

@carbolymer carbolymer Oct 30, 2024

Choose a reason for hiding this comment

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

Or we could expose lenses for ExecConfig for that purpose?

, "--out-file", txbodyFp
-- Create temporary HTTP server with files required by the call to `cardano-cli`
-- In this case, the server emulates an IPFS gateway
serveFilesWhile
Copy link
Contributor

Choose a reason for hiding this comment

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

What does a failure here look like?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

serveFilesWhile doesn't seem to interfere with the output in case of failure, you can see an example here: https://github.com/IntersectMBO/cardano-node/pull/6023/checks?check_run_id=32024584074

Is that what you meant?

Copy link
Contributor

@carbolymer carbolymer Oct 30, 2024

Choose a reason for hiding this comment

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

That failure is coming from line 127. Can you induce one inside the serveFilesWhile argument, e.g. in line 186?

@@ -0,0 +1,50 @@
Preamble

We, the zaniest inhabitants of the peculiar and bewildering land of Barataria, in honor of our illustrious Governor, Sancho Panza, renowned for his comically charming ordinances, do hereby present this Constitution to tickle your fancy and uphold the values of laughter, merriment, and the pursuit of hilarity for all our citizens.
Copy link
Contributor

Choose a reason for hiding this comment

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

Chat GPT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No idea, I downloaded it from the shortened link that was there before: https://tinyurl.com/2pahcy6z. It seems an old gist by @carloslodelar. @CarlosLopezDeLara, was that you? do you know the origin of it?

cardano-testnet/cardano-testnet.cabal Outdated Show resolved Hide resolved
@palas palas force-pushed the hash-check-in-transaction-build-tests branch 2 times, most recently from 25cd53a to 7e14eb8 Compare October 28, 2024 22:07
@palas palas requested review from a team as code owners October 29, 2024 11:05
@palas palas force-pushed the hash-check-in-transaction-build-tests branch 7 times, most recently from d641b5f to e3fb8f9 Compare October 29, 2024 17:22

H.note_ stderrOutput

H.assert ("Hashes do not match!" `Text.isInfixOf` Text.pack stderrOutput)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: use assertWith for more verbose failure reporting.

void $ execCli' execConfig
[ eraName, "stake-address", "registration-certificate"
, "--stake-verification-key-file", stakeVkeyFp
, "--key-reg-deposit-amt", show @Int 0 -- TODO: why this needs to be 0????
Copy link
Contributor

@carbolymer carbolymer Oct 30, 2024

Choose a reason for hiding this comment

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

This can't be 0 now. You can get the right value using:

getKeyDeposit :: (H.MonadAssertion m, MonadTest m, MonadIO m)

There's a helper function for creating stake key registration certs: https://github.com/IntersectMBO/cardano-node/blob/master/cardano-testnet/src/Testnet/Process/Cli/SPO.hs#L178

execConfig'
[ eraName, "governance", "action", "create-info"
, "--testnet"
, "--governance-action-deposit", show @Int 1_000_000 -- TODO: Get this from the node
Copy link
Contributor

@carbolymer carbolymer Oct 30, 2024

Choose a reason for hiding this comment

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

use getMinGovActionDeposit instead of hardcoding a value

@palas palas force-pushed the hash-check-in-transaction-build-tests branch from e3fb8f9 to 6aea891 Compare October 31, 2024 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FR] - Add File Hash Validation when Building Transaction
3 participants