Skip to content

Conversation

@palas
Copy link
Contributor

@palas palas commented Mar 5, 2025

Changelog

- description: |
    Modified Plutus cost calculation command to allow the user to supply offline data instead of querying the node
  type:
  - feature

Context

This PR addresses this issue: #945
This PR depends on the following PR from cardano-api, I would recommend looking at both in conjunction: IntersectMBO/cardano-api#771

How to trust this PR

The test should provide some confidence, there is not a lot of change in the actual implementation, except that the parameters allow to provide the required info directly instead of querying the node.

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

@palas palas self-assigned this Mar 5, 2025
@palas palas linked an issue Mar 5, 2025 that may be closed by this pull request
@palas palas marked this pull request as ready for review March 6, 2025 15:46
@palas palas force-pushed the query-interpreter branch 2 times, most recently from f98a9d8 to 885e91f Compare March 6, 2025 15:54
@palas palas force-pushed the query-interpreter branch from 838a241 to 685d8c8 Compare March 12, 2025 12:10
@palas palas requested a review from Jimbo4350 March 12, 2025 12:12
@palas palas force-pushed the query-interpreter branch 2 times, most recently from 690efe9 to 8a480f8 Compare April 10, 2025 16:03
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.

Nice work. After this round of commits and tidying up the commit history this is good to go.

@palas palas requested review from Jimbo4350 and carbolymer and removed request for carbolymer April 14, 2025 18:48
@palas palas force-pushed the query-interpreter branch from 19942ba to ad7b1a8 Compare April 15, 2025 18:57
@palas palas requested a review from newhoggy April 15, 2025 18:57
@palas palas force-pushed the query-interpreter branch from ad7b1a8 to 7078030 Compare April 15, 2025 18:59
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.

  • No need for the metavar changes to be in this PR. Please remove them to a separate PR and open an ADR.
  • This needs to be addresses: #1079 (comment)
  • Introduce online vs offline as pointed out in slack

Everything else looks good. Happy to approve after.

@palas palas force-pushed the query-interpreter branch from 7078030 to c93a53a Compare April 21, 2025 12:12
@palas palas requested a review from a team April 21, 2025 12:12
@palas palas requested a review from a team as a code owner April 21, 2025 12:12
@palas palas force-pushed the query-interpreter branch 2 times, most recently from 25e013e to 299861f Compare April 21, 2025 12:39
@palas palas requested a review from Jimbo4350 April 21, 2025 16:48
@Jimbo4350 Jimbo4350 self-requested a review April 22, 2025 13:59
palas added 4 commits April 25, 2025 18:18
Make `--out-file` optional

Add support for eras before conway
- Split online and offline functionality into subcommands
- Use point free style to improve code layout
- Replace `evalEitherM` and `readJsonFile` with `readJsonFileOk`
- Remove parametrisation on era

Co-authored-by: Mateusz Galazyn
<228866+carbolymer@users.noreply.github.com>
@palas palas force-pushed the query-interpreter branch from 5e415e4 to 6e22c35 Compare April 25, 2025 16:19
@palas palas dismissed carbolymer’s stale review April 25, 2025 16:20

Already addressed issues and agreed the PR was ready

@palas palas enabled auto-merge April 25, 2025 16:30
@palas palas added this pull request to the merge queue Apr 25, 2025
Merged via the queue into master with commit 8b8d15a Apr 25, 2025
25 checks passed
@palas palas deleted the query-interpreter branch April 25, 2025 17:46
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.

[FR] - Command to calculate the correct Plutus Script Costs offline

6 participants