-
Notifications
You must be signed in to change notification settings - Fork 37
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 GPv2Settlement.swap balances tests to Foundry #193
Conversation
Co-authored-by: mfw78 <53399572+mfw78@users.noreply.github.com>
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.
Looks good, though some nits on labelling / ordering
function test_performs_a_swap_from_erc20_to_erc20_balance_when_specified() public { | ||
performs_a_swap_with_the_specified_balances(GPv2Order.BALANCE_ERC20, GPv2Order.BALANCE_ERC20); | ||
} | ||
|
||
function test_performs_a_swap_from_internal_to_erc20_balance_when_specified() public { | ||
performs_a_swap_with_the_specified_balances(GPv2Order.BALANCE_INTERNAL, GPv2Order.BALANCE_ERC20); | ||
} | ||
|
||
function test_performs_a_swap_from_erc20_to_external_balance_when_specified() public { | ||
performs_a_swap_with_the_specified_balances(GPv2Order.BALANCE_ERC20, GPv2Order.BALANCE_EXTERNAL); | ||
} | ||
|
||
function test_performs_a_swap_from_internal_to_external_balance_when_specified() public { | ||
performs_a_swap_with_the_specified_balances(GPv2Order.BALANCE_INTERNAL, GPv2Order.BALANCE_EXTERNAL); | ||
} | ||
|
||
function test_performs_a_swap_from_erc20_to_internal_balance_when_specified() public { | ||
performs_a_swap_with_the_specified_balances(GPv2Order.BALANCE_ERC20, GPv2Order.BALANCE_INTERNAL); | ||
} | ||
|
||
function test_performs_a_swap_from_internal_to_internal_balance_when_specified() public { | ||
performs_a_swap_with_the_specified_balances(GPv2Order.BALANCE_INTERNAL, GPv2Order.BALANCE_INTERNAL); | ||
} |
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: I do find it a little odd to have the buyTokenBalance
defined first, as opposed to sellTokenBalance
(given the ordering seen in most places for the Order
struct, the logical path when conisidering token flow, and the definition in the original tests being sellTokenBalance
first).
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.
And on second look, the test labelling is reversed when considering from the perspective of token flow.
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 inverted the two.
Indeed in hindsight I'd invert the standard ordering to be buy then sell. Still, I've noticed when writing this test that I actually mixed up the two in the function input and only noticed and fixed it when running the test. So being consistent is better here.
Description
See title.
JS allows you to be much more flexible in tests, so this code is a workaround that's only reasonable because there are only few balance values.
Test Plan
CI.
Related Issues
#119