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.

Sign up for GitHub

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

Jump to bottom

reduce numOfTokens in OnRamp DA fuzz #137

Merged
merged 4 commits into from
Sep 15, 2023
Merged

reduce numOfTokens in OnRamp DA fuzz #137

merged 4 commits into from
Sep 15, 2023

Conversation

matYang
Copy link
Collaborator

@matYang matYang commented Sep 14, 2023

Motivation

when numberOfTokens in data availability fuzz tests reach uint64.max, with MESSAGE_BYTES_PER_TOKEN set to 128, the end result could reach 2^262 causing an arith overflow error. It was seen once today in CI.

Solution

cap numberOfTokens max value to uint32.max, this is still high enough for foreseeable future.

@matYang matYang marked this pull request as ready for review September 14, 2023 04:05
@github-actions
Copy link
Contributor

LCOV of commit c2dd36a during Solidity Foundry #550

Summary coverage rate:
  lines......: 98.9% (909 of 919 lines)
  functions..: 96.4% (189 of 196 functions)
  branches...: 91.1% (357 of 392 branches)

Files changed coverage rate: n/a

@matYang matYang changed the title reduce num of tokens in fuzz reduce numOfTokens in OnRamp DA fuzz Sep 14, 2023
RensR
RensR reviewed Sep 15, 2023
View reviewed changes
contracts/src/v0.8/ccip/test/onRamp/EVM2EVMOnRamp.t.sol
@@ -712,7 +712,7 @@ contract EVM2EVMOnRamp_getDataAvailabilityCostUSD is EVM2EVMOnRamp_getFeeSetup {
uint32 tokenTransferBytesOverhead
) public {
vm.assume(messageDataLength < type(uint64).max);
vm.assume(numberOfTokens < type(uint64).max);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's change the datatype of messageDataLength and numberOfTokens instead of assuming

also, prefer bound over assume for cases like these https://twitter.com/PaulRBerg/status/1622558791685242880

@github-actions
Copy link
Contributor

LCOV of commit cc2ace6 during Solidity Foundry #578

Summary coverage rate:
  lines......: 98.9% (909 of 919 lines)
  functions..: 96.4% (189 of 196 functions)
  branches...: 91.1% (357 of 392 branches)

Files changed coverage rate: n/a

@github-actions
Copy link
Contributor

LCOV of commit b349176 during Solidity Foundry #579

Summary coverage rate:
  lines......: 98.9% (909 of 919 lines)
  functions..: 96.4% (189 of 196 functions)
  branches...: 91.1% (357 of 392 branches)

Files changed coverage rate: n/a

@github-actions
Copy link
Contributor

LCOV of commit 9b39f2d during Solidity Foundry #580

Summary coverage rate:
  lines......: 98.9% (909 of 919 lines)
  functions..: 96.4% (189 of 196 functions)
  branches...: 91.1% (357 of 392 branches)

Files changed coverage rate: n/a

@matYang matYang merged commit 04cd86a into ccip-develop Sep 15, 2023
@matYang matYang deleted the fix-calldata-fuzz branch September 15, 2023 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RensR RensR RensR left review comments

@connorwstein connorwstein connorwstein approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@matYang @connorwstein @RensR