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

chore: Adding Core unit tests #55

Merged
merged 13 commits into from
Feb 8, 2024
Merged

chore: Adding Core unit tests #55

merged 13 commits into from
Feb 8, 2024

Conversation

dekz
Copy link
Member

@dekz dekz commented Jan 4, 2024

No description provided.

@dekz dekz marked this pull request as ready for review January 18, 2024 00:52
Copy link
Collaborator

@duncancmt duncancmt left a comment

Choose a reason for hiding this comment

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

Why does this PR have a gas-pessimizing effect?

src/core/Permit2Payment.sol Outdated Show resolved Hide resolved
test/unit/Utils.sol Outdated Show resolved Hide resolved
Comment on lines +119 to +120
// Broken usage of OtcOrderSettlement.OtcOrderFilled in 0.8.21
// https://github.com/foundry-rs/foundry/issues/6206
Copy link
Collaborator

Choose a reason for hiding this comment

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

Glad to see that this is fixed in 0.8.22. There's a bunch of places in this codebase and others where I ought to refactor

Comment on lines +44 to +48
_mockExpectCall(
address(TOKEN),
abi.encodeWithSelector(IERC20.allowance.selector, address(basic), address(POOL)),
abi.encode(amount)
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wish there was a way to do this in a "ask forgiveness, not permission" way. it seems so wasteful

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm on the fence, we could make it a default success dummy (instead of rejection dummy), but we do want to test that it checks the allowance, and we do want to test that when the allowance isn't set, that it does set the approval. At least in a few tests.

I don't entirely mind repeating ourselves here for the time being.

@duncancmt duncancmt merged commit 303954d into master Feb 8, 2024
4 checks passed
@duncancmt duncancmt deleted the jacob/unit-tests branch May 16, 2024 00:55
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.

2 participants