search

ethereum / execution-specs

Specification for the Execution Layer. Tracking network upgrades.
Creative Commons Zero v1.0 Universal
808 stars 222 forks source link

Moved Transaction types into their own files (eg. FeeMarketTransaction, LegacyTransaction) as mentioned in the issue assigned to me #894

Closed Redidacove closed 3 months ago

Redidacove commented 4 months ago

The fork_types modules are gonna increase in size in the future so I refactored transactions-related functions and variables in a separate module for all the forks

Related to Issue #

I created a separate file named transactions.py in the same directory as fork_types.py for different forks from arrow-glacier to tangerine-whistle. I moved the transaction-related content into transactions.py with the necessary imports and variables used in it .

Also, I created different commits for each fork for my accountability for which forks are done so far by me in another branch transaction_types_refactoring.

Cute Animal Picture

Thank you for allowing me to contribute to my dream org.😌

229ad628cbdf49c5a0ae302d2e59bdf4

SamWilsn commented 4 months ago

Not sure why GitHub isn't running the checks, but you've got a few problems with your typing/formatting/etc.

CONTRIBUTING.md has the full instructions, but to run the checks locally:

$ python3 -m venv venv
$ . venv/bin/activate
(venv) $ pip install tox
(venv) $ tox -e static

And to automatically fix a lot of errors:

$ python3 -m venv venv
$ . venv/bin/activate
(venv) $ pip install -e .[test,doc,lint]
(venv) $ isort src/
(venv) $ black src/
Redidacove commented 4 months ago

Actually I did changes after forking the repo and then made changes in separate branch ad then raised the PR where i am just confused about the fact that it is mentioned in the contribution.md that we need to clone original repo with recursive tag.Did I do anything wrong it I do it my way . Also should I fetch before pushing this time.

Redidacove commented 4 months ago

Now I have updated as per you said do check it out @SamWilsn

SamWilsn commented 4 months ago

i am just confused about the fact that it is mentioned in the contribution.md that we need to clone original repo with recursive tag.Did I do anything wrong it I do it my way

No your way is fine!

SamWilsn commented 4 months ago

I've fixed some of the easier errors and added docstrings. Please actually run the tooling, and fix the problems it reports. For example:

tests/berlin/test_rlp.py:5: error: Module "ethereum.berlin.fork_types" has no attribute "encode_transaction"  [attr-defined]
src/ethereum/homestead/fork.py:29: error: Module "ethereum.homestead.fork_types" has no attribute "TX_BASE_COST"  [attr-defined]
Redidacove commented 4 months ago

I am now running the tool it has lots of errors from modules that i even haven't touched yet what to do did all errors due to my changes ,do i need to fix them all .

Redidacove commented 4 months ago

Fixed the errors coming from my change plz review now @SamWilsn and thanks for fixing issues by your commit

Redidacove commented 4 months ago

Any updates on this @SamWilsn

SamWilsn commented 4 months ago

looking now.

SamWilsn commented 3 months ago

This is actually a bit more complicated than I thought. Using "Transaction" and "LegacyTransaction" (note the quotes) annotations breaks our RLP encoder. I have a couple ideas on how to fix it, but I'm not sure what the best option is. Let me get back to you.

Redidacove commented 3 months ago

Like is it something that happened due to my changes do let me know how can I help like I just wanna get this done.

SamWilsn commented 3 months ago

It's a result of your changes, but not your fault. Python doesn't convert the annotation from a string to a class, so we break somewhere around here:

https://github.com/ethereum/execution-specs/blob/0f9e4345b60d36c23fffaa69f70cf9cdb975f4ba/src/ethereum/rlp.py#L269

I haven't had a chance to think about it yet. Feel free to take a stab at it, but let me know your plan before you write a ton of code.

Redidacove commented 3 months ago

And finally, I can't comment directly on the line, but change:

transactions: Tuple[Union[Bytes, LegacyTransaction], ...]

To:

transactions: Tuple[Union[Bytes, "LegacyTransaction"], ...]

@SamWilsn any solution for this you have ,Do share I am not able to figure out how to fix this I can only think if we do it as it was previously without TYPE_CHECKING normal transaction and LegacyTransaction without " " (quotes) then it would work i believe. What do you think.

SamWilsn commented 3 months ago

we do it as it was previously without TYPE_CHECKING normal transaction and LegacyTransaction without " " (quotes) then it would work i believe. What do you think.

Unfortunately that would mean duplicating the definition of a few types like Address, right?

I still haven't sat down and tried any solutions yet, but I was thinking of maybe moving the block types into their own file (eg. src/ethereum/frontier/block.py.) I think that would resolve the circular dependency while not having to duplicate types.

I can't promise that'll work. I'll see if I can try it Monday.

Redidacove commented 3 months ago

okay sir 🫡

Redidacove commented 3 months ago

@SamWilsn any updates on this any solutions to the issue

Redidacove commented 3 months ago

@SamWilsn Did you got the time to look into the issue yet Ik you are super busy but can you just give some hint to me what solution you think to this can be so that I can do something I did a lot of hardship in this want this to complete hope you understand 😊😇.

SamWilsn commented 3 months ago

If you want to investigate it yourself, please feel free!

Best hint I can give right now is try keeping Address in fork_types.py and moving the block types to something like blocks.py.

The other approach would be fixing the RLP stuff to work with quoted types.

You'll have to try and see which one works.

On Thu, Mar 21, 2024, 13:49 Md Amaan @.***> wrote:

@SamWilsn https://github.com/SamWilsn Did you got the time to look into the issue yet Ik you are super busy but can you just give some hint to me what solution you think to this can be so that I can do something I did a lot of hardship in this want this to complete hope you understand 😊😇.

— Reply to this email directly, view it on GitHub https://github.com/ethereum/execution-specs/pull/894#issuecomment-2013166759, or unsubscribe https://github.com/notifications/unsubscribe-auth/ANU4EQPSREP4ZBC4E6HM3W3YZMMRNAVCNFSM6AAAAABDWBXLQ2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMJTGE3DMNZVHE . You are receiving this because you were mentioned.Message ID: @.***>

SamWilsn commented 3 months ago

Finally got the time to look at this. I've run the tests locally, so this should pass CI.

codecov-commenter commented 3 months ago

Codecov Report

Attention: Patch coverage is 94.67497% with 77 lines in your changes are missing coverage. Please review.

Project coverage is 74.26%. Comparing base (119208c) to head (74ebfbf).

Files Patch % Lines
src/ethereum/arrow_glacier/transactions.py 80.00% 14 Missing :warning:
src/ethereum/gray_glacier/transactions.py 80.00% 14 Missing :warning:
src/ethereum/arrow_glacier/fork.py 0.00% 4 Missing :warning:
src/ethereum/gray_glacier/fork.py 0.00% 4 Missing :warning:
src/ethereum/dao_fork/fork.py 0.00% 3 Missing :warning:
src/ethereum/muir_glacier/fork.py 0.00% 3 Missing :warning:
src/ethereum/arrow_glacier/bloom.py 0.00% 2 Missing :warning:
src/ethereum/arrow_glacier/vm/__init__.py 0.00% 2 Missing :warning:
src/ethereum/arrow_glacier/vm/interpreter.py 0.00% 2 Missing :warning:
src/ethereum/dao_fork/bloom.py 0.00% 2 Missing :warning:
... and 16 more
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #894 +/- ## ========================================== + Coverage 74.12% 74.26% +0.13% ========================================== Files 572 602 +30 Lines 32423 32670 +247 ========================================== + Hits 24034 24261 +227 - Misses 8389 8409 +20 ``` | [Flag](https://app.codecov.io/gh/ethereum/execution-specs/pull/894/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/ethereum/execution-specs/pull/894/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum) | `74.26% <94.67%> (+0.13%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Redidacove commented 3 months ago

Awesome I am so glad that it is done.