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

Modify estimateTransactionKeyWitnessCount to estimate simple scripts too #755

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

Modify estimateTransactionKeyWitnessCount to estimate simple scripts too #755

wants to merge 2 commits into from
+83 −6

Conversation

palas
Copy link
Contributor

@palas palas commented Feb 17, 2025 •
edited
Loading

Changelog

- description: |
    Modify `estimateTransactionKeyWitnessCount` to estimate simple scripts too
  type:
  - bugfix

Context

See #551

How to trust this PR

  • Make sure the test makes sense. I have run the test without changing the implementation and it does fail.
  • Ensure the implementation logic makes sense.
  • Check the code structure for good practices and enough documentation.

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 added the bug Something isn't working label Feb 17, 2025
@palas palas self-assigned this Feb 17, 2025
@palas palas linked an issue Feb 17, 2025 that may be closed by this pull request
[BUG] - estimateTransactionKeyWitnessCount doesn't appear to consider native (simple) scripts #551
Open
@palas palas force-pushed the fix-estimate-transaction-key-witness-count branch from 806a384 to 585a8f9 Compare February 18, 2025 15:33
@palas palas marked this pull request as ready for review February 18, 2025 15:44
carbolymer
carbolymer reviewed Feb 19, 2025
View reviewed changes
cardano-api/gen/Test/Gen/Cardano/Api/Typed.hs
genSimpleScript = genRandomSimpleScript True

genRandomSimpleScript :: Bool -> Gen SimpleScript
genRandomSimpleScript hasEmptyAnys=
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
genRandomSimpleScript hasEmptyAnys=
genRandomSimpleScript hasEmptyAnys =

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

@carbolymer carbolymer carbolymer approved these changes

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

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

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

@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

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

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

Assignees

@palas palas

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] - estimateTransactionKeyWitnessCount doesn't appear to consider native (simple) scripts
2 participants
@palas @carbolymer