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

Add command to calculate plutus script costs from an already constructed transaction #1031

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

Add command to calculate plutus script costs from an already constructed transaction #1031

wants to merge 24 commits into from

Conversation

Jimbo4350
Copy link
Contributor

@Jimbo4350 Jimbo4350 commented Jan 24, 2025 •
edited by palas
Loading

Changelog

- description: |
    Add command to calculate plutus script costs from an already constructed transaction.
  type:
  - feature

Context

This is a first step towards addressing this issue: #945.

This PR in cardano-api provides the required functionality: IntersectMBO/cardano-api#735. This PR must wait for the functionality in the cardano-api PR to get in a release, and the source repo stanza must be removed before merging.

How to trust this PR

Ensure the untyped bits are correct: serialisation, documentation, and interface design.

Check in conjunction with these PRs:

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated. See Running tests for more details
  • Self-reviewed the diff

@Jimbo4350 Jimbo4350 linked an issue Jan 24, 2025 that may be closed by this pull request
[FR] - Command to calculate the correct Plutus Script Costs offline #945
Open
Jimbo4350
Jimbo4350 commented Jan 24, 2025
View reviewed changes
Copy link
Contributor Author

@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.

If you look closely renderScriptCosts could be refactored to accept Map ScriptWitnessIndex ScriptHash as a parameter instead of [(ScriptWitnessIndex, AnyScriptWitness era)].

cardano-cli/src/Cardano/CLI/EraBased/Run/Transaction.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/EraBased/Run/Transaction.hs Outdated Show resolved Hide resolved
@palas palas self-assigned this Jan 24, 2025
@palas palas added the enhancement New feature or request label Jan 24, 2025
@palas palas changed the title Plutus script cost calculation cmd Add command to calculate plutus script costs from an already constructed transaction Jan 24, 2025
@palas palas mentioned this pull request Jan 24, 2025
Merged
3 tasks
@palas palas marked this pull request as ready for review January 24, 2025 16:06
carbolymer
carbolymer reviewed Jan 27, 2025
View reviewed changes
Copy link
Contributor

@carbolymer carbolymer 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.

Can you add a test case in cardano-testnet for that command?

cardano-cli/src/Cardano/CLI/EraBased/Run/Transaction.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/EraBased/Run/Transaction.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/EraBased/Commands/Transaction.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/EraBased/Run/Transaction.hs
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this makes --calculate-plutus-script-cost redundant. We should remove it or mark it as a deprecated and then remove it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Addressed here: 9548d1d

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice. I think we should print also a deprecation warning to stderr when using that option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

^^ 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Addressed here: 0d54cd9

@palas palas force-pushed the plutus-script-cost-calculation-cmd branch 2 times, most recently from 2a75785 to 34d3d17 Compare January 28, 2025 12:13
palas and others added 15 commits January 28, 2025 13:27
@palas palas force-pushed the plutus-script-cost-calculation-cmd branch from 34d3d17 to a639be4 Compare January 28, 2025 12:43
@palas palas force-pushed the plutus-script-cost-calculation-cmd branch from 27052c5 to 4d420e9 Compare January 30, 2025 16:16
@palas palas mentioned this pull request Jan 30, 2025
Open
9 tasks
carbolymer
carbolymer reviewed Jan 30, 2025
View reviewed changes
cardano-cli/src/Cardano/CLI/EraBased/Run/Transaction.hs
liftIO
$ ( case outputFile of
Just file -> LBS.writeFile (unFile file)
Nothing -> putLByteString
Copy link
Contributor

Choose a reason for hiding this comment

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

this should go to stderr

Copy link
Contributor

Choose a reason for hiding this comment

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

This is normal expected output. Is there a specific reason why you think it should go to stderr?

@palas palas removed a link to an issue Jan 31, 2025
[FR] - Command to calculate the correct Plutus Script Costs offline #945
Open
@palas palas mentioned this pull request Jan 31, 2025
Open
@palas palas linked an issue Jan 31, 2025 that may be closed by this pull request
Command to calculate the correct Plutus Script Costs online #1045
Open
@palas palas force-pushed the plutus-script-cost-calculation-cmd branch from ff8b1b1 to 6debdee Compare January 31, 2025 15:40
@palas palas force-pushed the plutus-script-cost-calculation-cmd branch from 874c63c to 0d54cd9 Compare January 31, 2025 17:13
@palas palas force-pushed the plutus-script-cost-calculation-cmd branch 2 times, most recently from d80e63d to 8bd3765 Compare January 31, 2025 18:13
@palas palas force-pushed the plutus-script-cost-calculation-cmd branch from 8bd3765 to 3604f0c Compare January 31, 2025 18:16
Jimbo4350
Jimbo4350 commented Feb 4, 2025
View reviewed changes
Copy link
Contributor Author

@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! Feel free to close this PR and re-open a new one so it is rightly counted as yours.

Oh and clean up the git history please!

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

@palas palas palas left review comments

@carbolymer carbolymer carbolymer approved these changes

@newhoggy newhoggy Awaiting requested review from newhoggy newhoggy is a code owner

@smelc smelc Awaiting requested review from smelc smelc is a code owner

@kevinhammond kevinhammond Awaiting requested review from kevinhammond kevinhammond is a code owner

@disassembler disassembler Awaiting requested review from disassembler disassembler is a code owner

Assignees

@palas palas

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Command to calculate the correct Plutus Script Costs online
3 participants
@Jimbo4350 @carbolymer @palas