-
Notifications
You must be signed in to change notification settings - Fork 35
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
chore: migrate E2E wineOilMarket tests to Foundry #214
chore: migrate E2E wineOilMarket tests to Foundry #214
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The migrated test looks good, just a few comments on keeping the code easier to maintain in the future.
0921f74
to
66991a0
Compare
tokens[1] = oil; | ||
tokens[2] = wine; | ||
uint256[] memory prices = new uint256[](3); | ||
prices[0] = 1 ether; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: for readability, maybe allocate to a variable per oilPrice
and winePrice
unless there's stack too deep issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i dont understand what you mean
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is odd to have oilPrice
and winePrice
and not have eurPrice
. So, at LN108 I'd suggest adding a eurPrice
variable.
3f45967
to
9fed3fc
Compare
Description
Migrate wineOilMarket e2e test to foundry. Depends on #215 for the
ForkedTest.t.sol
.Test Plan
CI
Related Issues
Closes #143