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

fix: precompile revert #24

Merged
merged 2 commits into from
Oct 11, 2024
Merged

Conversation

dudong2
Copy link
Collaborator

@dudong2 dudong2 commented Oct 7, 2024

Closes: #XXX

Description


For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@dudong2 dudong2 self-assigned this Oct 7, 2024
@cloudgray cloudgray self-requested a review October 8, 2024 06:29
Copy link
Collaborator

@cloudgray cloudgray left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@dongsam dongsam left a comment

Choose a reason for hiding this comment

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

How about adding unit tests for the Revert, ProcessPrecompileEvents, and AddPrecompileSnapshot? Although there are integration tests at the app level, these functions are critical and could have severe consequences if they behave even slightly differently than intended, so having unit tests would be beneficial.

Copy link
Collaborator

@zsystm zsystm left a comment

Choose a reason for hiding this comment

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

LGTM for changes.

@dudong2
Copy link
Collaborator Author

dudong2 commented Oct 10, 2024

How about adding unit tests for the Revert, ProcessPrecompileEvents, and AddPrecompileSnapshot? Although there are integration tests at the app level, these functions are critical and could have severe consequences if they behave even slightly differently than intended, so having unit tests would be beneficial.

For the ProcessPrecompileEvents function, it is not a critical component. The key is the revert functionality through the precompileChange journal. To address this, I have added tests that mimic the actual flow of calling the precompiled contract from a contract.
If this level of testing is sufficient, I will proceed with merging this pr.

@dongsam dongsam self-requested a review October 11, 2024 03:40
@dudong2 dudong2 merged commit 8eb8846 into basechain/abci2.0/develop Oct 11, 2024
18 checks passed
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.

4 participants