Skip to content
This repository has been archived by the owner on Nov 28, 2021. It is now read-only.

Added cancelWithdrawRequest and tests #51

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Added cancelWithdrawRequest and tests #51

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a3ce027
Added cancelWithdrawRequest and tests
JonathanStefanov Sep 17, 2021
30a846e
Added requirements for fulfillWithdrawRequest function
JonathanStefanov Sep 18, 2021
805d132
Corrections
JonathanStefanov Oct 10, 2021
4c1ed16
Revert "Added requirements for fulfillWithdrawRequest function"
JonathanStefanov Oct 10, 2021
488ebbe
Second correction
JonathanStefanov Oct 10, 2021
1a183d5
Changed require in cancelWithdrawRequest method
JonathanStefanov Oct 10, 2021
120f773
Delete env
JonathanStefanov Oct 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 9 additions & 3 deletions contracts/RenPool.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,8 +178,8 @@ contract RenPool {
function fulfillWithdrawRequest(address _target) external {
address sender = msg.sender;
uint amount = withdrawRequests[_target];
// ^ This could not be defined plus make sure amount > 0
// TODO: make sure user cannot fullfil his own request
require(amount > 0, "Amount has to be positive");
require(sender != _target, "Sender cannot be yourself");
// TODO: add test for when _target doesn't have an associated withdrawRequest
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think the third todo is automatically implemented when we test if the amount is positive, what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure if I follow. What I was trying to say is: in case we have this situation _target doesn't have an associated withdrawRequest then the code should revert. I think your code implements this situation.

Now, what I was referring was to add an actual test to the test suit to cover this case

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh right, I thought you meant checking it in the smart contract, my bad.
Will definitely implement the test yes

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will do this in a later PR since it is not related to cancelWithdrawRequest


require(isLocked == true, "Pool is not locked");
Expand All @@ -201,7 +201,13 @@ contract RenPool {
// TODO emit event
}

// TODO: cancelWithdrawRequest
function cancelWithdrawRequest() external {
address sender = msg.sender;
// Strictly positive so that we are sure not to have error in case the withdraw request doesn't exist for example
require(withdrawRequests[sender] > 0, "No withdraw request");

Copy link
Member

Choose a reason for hiding this comment

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

I'm hesitating if we need the first 2 lines or not. Also, It'd be nice to add tests for this piece of code as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll add the tests, as for the lines sender is used two times in the function so making it a variable makes sence in my opinion.
I could also use msg.sender too of course

Copy link
Member

Choose a reason for hiding this comment

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

I mean, not sure if we need the require statement here.

delete withdrawRequests[sender];
}
// TODO: getWithdrawRequests

/**
Expand Down
28 changes: 28 additions & 0 deletions tests/ren_pool/test_withdraw_cancellation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
from brownie.test import given, strategy
from brownie import accounts
import pytest
import constants as C

@pytest.mark.parametrize('user', accounts[0:3]) # [owner, nodeOperator, user]
@given(
amount=strategy('uint256', min_value = 1, max_value = C.POOL_BOND),
)
def test_ren_pool_withdraw_cancellation(ren_pool, ren_token, user, amount, owner):
"""
Test withdraw cancellation happy path.
"""
# Owner locks pool (could be any other user)
ren_token.approve(ren_pool, C.POOL_BOND, {'from': owner})
ren_pool.deposit(C.POOL_BOND, {'from': owner})

Copy link
Member

Choose a reason for hiding this comment

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

We could use user instead of owner to be more generic

# The pool is locked. We can now request withdraw
ren_pool.requestWithdraw(amount, {'from': user})

# Make sure the withdraw request exists
assert ren_pool.withdrawRequests(user) == amount

# Delete the withdraw request
ren_pool.cancelWithdrawRequest({'from': user})

# Make sure the withdraw request does not exist anymore
assert ren_pool.withdrawRequests(user) == None