Skip to content

Conversation

paterson1
Copy link
Contributor

@paterson1 paterson1 commented Nov 9, 2022

Context

Closes #390

Changes proposed in this pull request

Refactored out some common constants between placeOrder and closeOrder. Also tried to flatten the nesting of describe blocks without losing too much context for each unit test.

Checklist

  • Rebased the base branch
  • Attached corresponding Github issue
  • Prefixed the name with the type of change (i.e. feat, chore, test)
  • Performed a self-review of my own code
  • Followed the style guidelines of this project
  • Made corresponding changes to the documentation
  • Didn't generate new warnings
  • Didn't generate failures on existing tests
  • Added tests that prove my fix is effective or that my feature works

Review

When reviewing a PR, please indicate intention in comments using the following emojis:

  • 🍰 = Nice to have but not essential.
  • 💡 = Suggestion or a comment based on personal opinion
  • 🔨 = I believe this should be changed.
  • 🤔 = I don’t understand something, do you mind giving me more context?
  • 🚀 = Feedback

@paterson1 paterson1 requested a review from patitonar November 9, 2022 22:02
@roxdanila roxdanila linked an issue Nov 10, 2022 that may be closed by this pull request
@paterson1 paterson1 marked this pull request as ready for review November 10, 2022 19:31
@paterson1 paterson1 marked this pull request as draft November 10, 2022 19:36
@paterson1 paterson1 force-pushed the test/swapOperator-tests-refactor branch from 54b532b to 9ee07fc Compare November 10, 2022 19:37
@paterson1 paterson1 marked this pull request as ready for review November 10, 2022 19:39
// Executing as non-controller should succeed
await setNextBlockTime(deadline + 1);
await swapOperator.connect(governance).closeOrder(contractOrder);
// Executing as non-controller should succeed
Copy link
Contributor

Choose a reason for hiding this comment

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

let's make this address some random address not governance so the test is accurate

.to.emit(swapOperator, 'OrderClosed')
.withArgs(makeOrderTuple(contractOrder), order.sellAmount.div(2));
});
it('emitting OrderClosed event when order was partially filled', async function () {
Copy link
Contributor

@danoctavian danoctavian Nov 11, 2022

Choose a reason for hiding this comment

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

emits OrderClosed event when order was partially filled


it('when order was fully filled', async function () {
await cowSettlement.fill(contractOrder, orderUID, order.sellAmount, order.feeAmount, order.buyAmount);
it('emitting OrderClosed event when order was fully filled', async function () {
Copy link
Contributor

@danoctavian danoctavian Nov 11, 2022

Choose a reason for hiding this comment

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

emits OrderClosed event when order was fully filled

expect(await weth.allowance(swapOperator.address, cowVaultRelayer.address)).to.eq(
order.sellAmount.add(order.feeAmount),
);
it('canceling the presignature and allowance: does so if the order was not filled at all', async function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

'presignature is false and allowance is 0 when order was not filled at all'

order.feeAmount.div(2),
order.buyAmount.div(2),
);
it('canceling the presignature and allowance: does so if the order is partially filled', async function () {
Copy link
Contributor

@danoctavian danoctavian Nov 11, 2022

Choose a reason for hiding this comment

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

'cancels presignature and allowance when the order is partially filled'

);
expect(await dai.balanceOf(swapOperator.address)).to.eq(0);
expect(await weth.balanceOf(swapOperator.address)).to.gt(0);
it('canceling the presignature and allowance: does so if the order was fully filled', async function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

'cancels presignature and allowance when the order is fully filled'

it('withdraws and unwraps ether if buyToken is weth', async function () {
// Cancel current order
await swapOperator.closeOrder(contractOrder);
it('withdrawing buyToken to pool withdraws and unwraps ether if buyToken is weth', async function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

withdraws buyToken to pool and unwraps ether if buyToken is weth

it('transfer the erc20 token if its not weth', async function () {
expect(await dai.balanceOf(swapOperator.address)).to.eq(0);
expect(await dai.balanceOf(pool.address)).to.eq(0);
it('withdrawing buyToken to pool transfer the erc20 token if its not weth', async function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

withdraws buyToken to pool when buyToken is an erc20 token

it('withdraws and unwraps ether if sellToken is weth', async function () {
const initialPoolEth = await ethers.provider.getBalance(pool.address);
const initialOperatorWeth = await weth.balanceOf(swapOperator.address);
it('returning sellToken to pool withdraws and unwraps ether if sellToken is weth', async function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

returns sellToken to pool and unwraps ether if sellToken is weth

it('transfers the erc20 token to pool if its not weth', async function () {
// Cancel current order
await swapOperator.closeOrder(contractOrder);
it('returning sellToken to pool transfers the erc20 token to pool if its not weth', async function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

returns sellToken to pool when sellToken is an erc20 token

.to.emit(swapOperator, 'OrderClosed')
.withArgs(makeOrderTuple(contractOrder), 0);
});
it('when order was not filled', async function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

emits OrderClosed event when order was not filled

it('performs the validation when buyToken is not WETH', async function () {
// Since DAI was already registered on setup, set its details to 0
await pool.connect(governance).setSwapDetails(dai.address, 0, 0, 0, true); // otherSigner is governant
it('performs the validation assets details when buyToken is not WETH', async function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

validates asset details when buyToken is not WETH

lastSwapTime = (await pool.getAssetSwapDetails(dai.address)).lastSwapTime;
expect(lastSwapTime).to.eq(await lastBlockTimestamp());
});
it('setting lastSwapDate: sets it on buyAsset when selling ETH', async function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

sets lastSwapDate on buyAsset when selling ETH

describe('setting pools swapValue', function () {
it('works when selling eth', async function () {
expect(await pool.swapValue()).to.eq(0);
it('setting lastSwapDate: sets it on sellAsset when buying ETH', async function () {
Copy link
Contributor

@danoctavian danoctavian Nov 11, 2022

Choose a reason for hiding this comment

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

sets lastSwapDate on sellAsset when buying ETH

Copy link
Contributor

@danoctavian danoctavian left a comment

Choose a reason for hiding this comment

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

Test description renames required

Copy link
Contributor

@danoctavian danoctavian left a comment

Choose a reason for hiding this comment

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

LGTM

@roxdanila roxdanila force-pushed the test/swapOperator-tests-refactor branch from 3772821 to f7a00c6 Compare November 15, 2022 10:24
@roxdanila roxdanila merged commit ed52739 into nexus-v2 Nov 15, 2022
@roxdanila roxdanila deleted the test/swapOperator-tests-refactor branch November 15, 2022 10:35
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.

Cleanup CowSwap unit tests
3 participants