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

Merged
merged 5 commits into from
Feb 21, 2025
Merged

Modify estimateTransactionKeyWitnessCount to estimate simple scripts too #755

merged 5 commits into from
Feb 21, 2025

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
Closed
@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
Jimbo4350
Jimbo4350 approved these changes Feb 20, 2025
View reviewed changes
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. Have a look at the comments.

@palas palas added this pull request to the merge queue Feb 21, 2025
Merged via the queue into master with commit ba6003d Feb 21, 2025
29 checks passed
@palas palas deleted the fix-estimate-transaction-key-witness-count branch February 21, 2025 11:10
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

@Jimbo4350 Jimbo4350 Jimbo4350 approved these changes

@dcoutts dcoutts Awaiting requested review from dcoutts dcoutts 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
3 participants
@palas @carbolymer @Jimbo4350