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): Proper handling of fallback and receive functions in common precompile #2064

Merged
merged 14 commits into from Nov 21, 2023

Conversation

Vvaradinov
Copy link
Contributor

@Vvaradinov Vvaradinov commented Nov 21, 2023

Description


After a few more integration tests and the discussion with @VictorTrustyDev here - #2061 the original solution to handling receive and fallback methods was very naive. This PR aims to fix that by taking into consideration all possible cases possible while maintaining a nice solution that plays well with all past and potential future precompiles that may need to define any of the 2 functions. Some notes on the implementation:

  • The cases were adapted directly from the official Solidity docs - fallback function and receive function
  • We are identifying these functions with the methods HasFallback and HasReceive. This is why this solution is future proof and backwards compatible with our existing precompiles as they do not define any of these functions in their ABI so the logic will be skipped.
  • The solution assumes we will always define both functions as payable if we decide that we might be mixing these in the future with non payable additional cases must be created CC @fedekunze

Thanks again @VictorTrustyDev for flagging this.

Closes #XXX

@Vvaradinov Vvaradinov requested a review from a team as a code owner November 21, 2023 10:51
@Vvaradinov Vvaradinov requested review from facs95, GAtom22 and fedekunze and removed request for a team November 21, 2023 10:51
precompiles/common/precompile.go Outdated Show resolved Hide resolved
precompiles/werc20/werc20.go Outdated Show resolved Hide resolved
Copy link

codecov bot commented Nov 21, 2023

Codecov Report

Merging #2064 (828030e) into main (9c01f06) will decrease coverage by 0.11%.
The diff coverage is 0.00%.

❗ Current head 828030e differs from pull request most recent head abcdc56. Consider uploading reports for the commit abcdc56 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2064      +/-   ##
==========================================
- Coverage   70.16%   70.06%   -0.11%     
==========================================
  Files         340      340              
  Lines       25455    25492      +37     
==========================================
  Hits        17861    17861              
- Misses       6676     6713      +37     
  Partials      918      918              
Files Coverage Δ
precompiles/common/precompile.go 0.00% <0.00%> (ø)

Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

Left comments. It's hard to follow the cases and the implementation can be cleaned up.

We also need to write tests for each of the cases

precompiles/common/precompile.go Outdated Show resolved Hide resolved
precompiles/common/precompile.go Outdated Show resolved Hide resolved
precompiles/common/precompile.go Outdated Show resolved Hide resolved
precompiles/common/precompile.go Outdated Show resolved Hide resolved
precompiles/common/precompile.go Outdated Show resolved Hide resolved
precompiles/common/precompile.go Outdated Show resolved Hide resolved
precompiles/common/precompile.go Outdated Show resolved Hide resolved
precompiles/common/precompile.go Outdated Show resolved Hide resolved
precompiles/common/precompile.go Outdated Show resolved Hide resolved
precompiles/common/precompile.go Outdated Show resolved Hide resolved
precompiles/common/precompile.go Outdated Show resolved Hide resolved
precompiles/common/precompile.go Outdated Show resolved Hide resolved
precompiles/common/precompile.go Outdated Show resolved Hide resolved
precompiles/werc20/werc20.go Outdated Show resolved Hide resolved
@fedekunze fedekunze merged commit 6b89f64 into main Nov 21, 2023
15 of 24 checks passed
@fedekunze fedekunze deleted the Vvaradinov/detailed-fallback-receive-handling branch November 21, 2023 20:38
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.

None yet

3 participants