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.

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

dev: use with_base_fee for TransactionInfo #1069

Closed
tcoratger opened this issue May 10, 2024 · 6 comments · Fixed by #1201
Closed

dev: use with_base_fee for TransactionInfo #1069

tcoratger opened this issue May 10, 2024 · 6 comments · Fixed by #1201
Assignees
Labels
blocked Blocked right now for external reasons enhancement Enhancement of the code, not introducing new features. good first issue Good first issue for people wanting to contribute to this project.

Comments

@tcoratger
Copy link
Collaborator

Describe the enhancement request

When alloy-rs/alloy#721 is included in the next release of reth (blocker at the moment), these lines

let mut transaction_info = TransactionInfo::from(tx);
transaction_info.base_fee = Some(block_base_fee);
could be merged into a single one using the dedicated with_base_fee method that consumes the TransactionInfo instance and populates it using the provided base_fee.

@tcoratger tcoratger added enhancement Enhancement of the code, not introducing new features. good first issue Good first issue for people wanting to contribute to this project. labels May 10, 2024
@github-project-automation github-project-automation bot moved this to 🆕 Backlog in Kakarot on Starknet May 10, 2024
@Gerson2102
Copy link
Contributor

Hello. Can i take this?

@tcoratger
Copy link
Collaborator Author

Hello. Can i take this?

Assigned

@Gerson2102
Copy link
Contributor

@tcoratger hello sir. When this is included in the next release of reth, can you ping to work in this issue like the last time?

@tcoratger
Copy link
Collaborator Author

@tcoratger hello sir. When this is included in the next release of reth, can you ping to work in this issue like the last time?

Yes sure

@greged93 greged93 added the blocked Blocked right now for external reasons label May 30, 2024
@Bosun-Josh121
Copy link

Pls assign, can complete within a day or less

@tcoratger
Copy link
Collaborator Author

Pls assign, can complete within a day or less

Assigned

@github-project-automation github-project-automation bot moved this from 🆕 Backlog to ✅ Done in Kakarot on Starknet Jun 17, 2024
anukkrit149 pushed a commit to karnotxyz/kakarot-rpc that referenced this issue Aug 9, 2024
Made coinbase storage var a constructor argument rather than a hardcoded
value.

Time spent on this PR: 1 hour
(methodology: searched the whole source tree for "coinbase", "0xCA40",
"precompiles", and "uninitialized")

## Pull request type

- [ ] Bugfix
- [ ] Feature
- [ ] Code style update (formatting, renaming)
- [X] Refactoring (no functional changes, no api changes)
- [ ] Build related changes
- [ ] Documentation content changes
- [ ] Other (please describe):

## What is the current behavior?

Currently Coinbase is a hardcoded constant value
(0xCA40796aFB5472abaeD28907D5ED6FC74c04954a) duplicated in a few places
in the .py and .cairo sources.

Resolves kkrt-labs#1011

## What is the new behavior?

That constant is now defined in a single location in constants.py and
imported as needed. Accordingly, codebase is also passed as a new
argument in the "kakarot" and "EVM" constructors.

<!-- Reviewable:start -->
- - -
This change is [<img src="https://reviewable.io/review_button.svg"
height="34" align="absmiddle"
alt="Reviewable"/>](https://reviewable.io/reviews/kkrt-labs/kakarot/1069)
<!-- Reviewable:end -->

---------

Co-authored-by: Rene Doursat <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Blocked right now for external reasons enhancement Enhancement of the code, not introducing new features. good first issue Good first issue for people wanting to contribute to this project.
Projects
No open projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants