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

Refactor includability check in forks #890

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

Conversation

richardgreg
Copy link
Contributor

@richardgreg richardgreg commented Feb 22, 2024

What was wrong?

Includability checks should be moved from process_transaction() to check_transaction()

Related to Issue #
Ref #799

How was it fixed?

  • Moved the check code block from process_transaction() to check_transaction()
  • Added the necessary function argument to check_transaction

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

@richardgreg
Copy link
Contributor Author

Some process_transaction functions have ensure(validate_transaction(tx), InvalidBlock). Thought about moving them to process transaction too. What are your thoughts?

@richardgreg richardgreg changed the title [WIP]Refactor includability check in forks Refactor includability check in forks Feb 28, 2024
@richardgreg
Copy link
Contributor Author

@petertdavies could you take a quick look and state if I'm on the right path?

@richardgreg
Copy link
Contributor Author

@SamWilsn could you review this if you have the time? 🙂

@SamWilsn
Copy link
Collaborator

I'll do my best! Trying to get #925 merged before we start refactoring in earnest.

@gurukamath
Copy link
Collaborator

gurukamath commented Apr 22, 2024

@richardgreg There might be a few more checks that we might have to move to check_transaction. I have raised a PR to make these changes in cancun (See PR). You could use a similar template to port the changes over to the other forks.

@richardgreg
Copy link
Contributor Author

Awesome

@richardgreg
Copy link
Contributor Author

Hi @gurukamath, I just dealt with some conflicts while performing a rebase. I will get around to adding the checks with your template as soon as possible, but if this PR is okay, I'll appreciate it if you merge it so I don't have to rebase again when I get back to the task 😅 🙏🏾

@richardgreg
Copy link
Contributor Author

Does anyone know if these changes are still needed? I noticed it was approved two weeks ago but was never merged and now has a lot of conflicts

@SamWilsn
Copy link
Collaborator

@Bbjj88h is a spam bot as far as I can tell.

@SamWilsn
Copy link
Collaborator

Ah, sorry about the conflicts. We've removed the ensure function because it was hiding lines from code coverage in #932 & #944. I've assigned this to @petertdavies to walk you through and get this merged.

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.

5 participants