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.

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

evaluateTransactionExecutionUnitsShelley: return logs #555

Merged
merged 2 commits into from
Jun 18, 2024
Merged

evaluateTransactionExecutionUnitsShelley: return logs #555

merged 2 commits into from
Jun 18, 2024

Conversation

smelc
Copy link
Contributor

@smelc smelc commented Jun 14, 2024

Changelog

- description: |
    evaluateTransactionExecutionUnitsShelley: return logs that are useful for debugging
# uncomment types applicable to the change:
  type:
  # - feature        # introduces a new feature
  - breaking       # the API has changed in a breaking way
  # - compatible     # the API has changed but is non-breaking
  # - optimisation   # measurable performance improvements
  # - refactoring    # QoL changes
  # - bugfix         # fixes a defect
  # - test           # fixes/modifies tests
  # - maintenance    # not directly related to the code
  # - release        # related to a new release preparation
  # - documentation  # change in code docs, haddocks...

Context

This was asked by a user here on Slack and it makes sense, since existing callers can just ignore the new logs that are being returned; taking advantage of Haskell's lazyness.

How to trust this PR

It's returning more data

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • Self-reviewed the diff

@smelc smelc force-pushed the smelc/more-logging-in-evaluateTransactionExecutionUnit branch from 0e3e5cf to 8baad9b Compare June 14, 2024 08:38
palas
palas approved these changes Jun 14, 2024
View reviewed changes
Copy link
Contributor

@palas palas left a comment

Choose a reason for hiding this comment

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

You're right, it looks like evalTxExUnits just uses evalTxExUnitsWithLogs anyway.
I just wrote a stylistic suggestion.

cardano-api/internal/Cardano/Api/Fees.hs Outdated Show resolved Hide resolved
Jimbo4350
Jimbo4350 requested changes Jun 14, 2024
View reviewed changes
Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice 👍 . I agree with @palas's comment.

cardano-api/internal/Cardano/Api/Fees.hs Outdated
@@ -631,11 +631,11 @@ evaluateTransactionExecutionUnitsShelley :: forall era. ()
-> UTxO era
-> L.Tx (ShelleyLedgerEra era)
-> Either (TransactionValidityError era)
(Map ScriptWitnessIndex (Either ScriptExecutionError ExecutionUnits))
(Map ScriptWitnessIndex (Either ScriptExecutionError ([Text.Text], ExecutionUnits)))
Copy link
Contributor

Choose a reason for hiding this comment

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

A type annotation isn't strictly necessary but it wouldn't hurt here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Jimbo4350> do you mean something like:

type TransactionExecutionUnits = Map ScriptWitnessIndex (Either ScriptExecutionError ([Text], ExecutionUnits))

and using TransactionExecutionUnits in evaluateTransactionExecutionUnits?

Copy link
Contributor

Choose a reason for hiding this comment

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

For [Text.Text]. The rest of the signature is fine.

@smelc smelc force-pushed the smelc/more-logging-in-evaluateTransactionExecutionUnit branch from 8baad9b to e881c2d Compare June 17, 2024 07:43
@Jimbo4350 Jimbo4350 self-requested a review June 17, 2024 10:10
Jimbo4350
Jimbo4350 reviewed Jun 17, 2024
View reviewed changes
cardano-api/internal/Cardano/Api/Fees.hs Outdated
@@ -618,7 +619,7 @@ evaluateTransactionExecutionUnits :: forall era. ()
-> UTxO era
-> TxBody era
-> Either (TransactionValidityError era)
(Map ScriptWitnessIndex (Either ScriptExecutionError ExecutionUnits))
(Map ScriptWitnessIndex (Either ScriptExecutionError ([Text], ExecutionUnits)))
Copy link
Contributor

Choose a reason for hiding this comment

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

A type synonym for [Text] would aid in readability

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Jimbo4350> added type synonym EvalTxExecutionUnitsLog.

@smelc smelc force-pushed the smelc/more-logging-in-evaluateTransactionExecutionUnit branch from b3fc10b to 854347a Compare June 17, 2024 15:53
Jimbo4350
Jimbo4350 reviewed Jun 18, 2024
View reviewed changes
cardano-api/internal/Cardano/Api/Fees.hs Outdated
(Either (L.TransactionScriptFailure (ShelleyLedgerEra era)) Alonzo.ExUnits)
-> Map ScriptWitnessIndex (Either ScriptExecutionError ExecutionUnits)
(Either (L.TransactionScriptFailure (ShelleyLedgerEra era)) ([Text.Text], Alonzo.ExUnits))
-> Map ScriptWitnessIndex (Either ScriptExecutionError ([Text.Text], ExecutionUnits))
Copy link
Contributor

Choose a reason for hiding this comment

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

Here as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 👍

Jimbo4350
Jimbo4350 reviewed Jun 18, 2024
View reviewed changes
cardano-api/internal/Cardano/Api/Fees.hs
fromLedgerScriptExUnitsMap aOnwards exmap =
Map.fromList
[ (toScriptIndex aOnwards rdmrptr,
bimap (fromAlonzoScriptExecutionError aOnwards) fromAlonzoExUnits exunitsOrFailure)
bimap (fromAlonzoScriptExecutionError aOnwards) (second fromAlonzoExUnits) exunitsOrFailure)
Copy link
Contributor

Choose a reason for hiding this comment

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

You can actually use fmap here instead of second.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I rather not generalize this code that is involved already. I think second better conveys the intent 👍

@smelc smelc force-pushed the smelc/more-logging-in-evaluateTransactionExecutionUnit branch from 854347a to 37fae81 Compare June 18, 2024 08:37
@smelc smelc enabled auto-merge June 18, 2024 08:38
@smelc smelc added this pull request to the merge queue Jun 18, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Jun 18, 2024
@smelc smelc merged commit 850aa19 into main Jun 18, 2024
23 checks passed
@smelc smelc deleted the smelc/more-logging-in-evaluateTransactionExecutionUnit branch June 18, 2024 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jimbo4350 Jimbo4350 Jimbo4350 approved these changes

@palas palas palas approved these changes

@dcoutts dcoutts Awaiting requested review from dcoutts

@erikd erikd Awaiting requested review from erikd

@newhoggy newhoggy Awaiting requested review from newhoggy

@carbolymer carbolymer Awaiting requested review from carbolymer

@kevinhammond kevinhammond Awaiting requested review from kevinhammond

@CarlosLopezDeLara CarlosLopezDeLara Awaiting requested review from CarlosLopezDeLara

@disassembler disassembler Awaiting requested review from disassembler

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@smelc @palas @Jimbo4350