contracts-bedrock: portal opaque data test#10443
Conversation
WalkthroughWalkthroughThe recent update focuses on enhancing the OptimismPortal test suite in the Changes
Recent Review DetailsConfiguration used: .coderabbit.yml Files selected for processing (1)
Additional comments not posted (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #10443 +/- ##
============================================
- Coverage 42.31% 29.22% -13.09%
============================================
Files 73 31 -42
Lines 4838 2898 -1940
Branches 766 614 -152
============================================
- Hits 2047 847 -1200
+ Misses 2682 1976 -706
+ Partials 109 75 -34
Flags with carried forward coverage won't be shown. Click here to find out more. |
geoknee
left a comment
geoknee
left a comment
There was a problem hiding this comment.
LGTM, left one suggestion.
tynes
force-pushed
the
ctb/portal-opaque-data
branch
from
bafd7c4 to
bdf4b7e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (1)
packages/contracts-bedrock/test/L1/OptimismPortal.t.sol (1)
Line range hint
741-741: Incorrect usage ofblockhash(block.number).The use of
blockhash(block.number)will always return 0, which is likely not the intended behavior. Consider using a different block number that is within the last 256 blocks.- blockhash(block.number) + blockhash(block.number - 1) // Assuming the previous block's hash is required
Adds deposit event differential coverage for the two different places where the deposit tx event exists. This ensures that a valid event ends up getting emitted. Also modularizes the code for serializing the event so that there are not footguns when computing the "opaque data".
Removes the diff to the portal, perhaps we want it but test still passes without it
tynes
force-pushed
the
ctb/portal-opaque-data
branch
from
d18c90a to
a71528e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (1)
packages/contracts-bedrock/test/L1/OptimismPortal.t.sol (1)
Line range hint
743-743: Incorrect use ofblockhash(block.number)in testing environment.- blockhash(block.number) + blockhash(block.number - 1)The use of
blockhash(block.number)always returns 0 in the Ethereum environment, which can lead to incorrect test outcomes. It should be replaced withblockhash(block.number - 1)to fetch the hash of the most recently mined block.
3 participants
Description
contracts-bedrock: add deposit event coverage
Adds deposit event differential coverage for the two
different places where the deposit tx event exists.
This ensures that a valid event ends up getting emitted.
A commit 372ec6b
is reverted that modularizes the code for serializing
the event so that there are not footguns when computing
the "opaque data". We can bring this back easily if
we would like.