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

Conversation

JonathanStefanov
Copy link
Collaborator

No description provided.

// ^ 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

// ^ 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
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

function cancelWithdrawRequest() external {
address sender = msg.sender;
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.

Test withdraw cancellation happy path.
"""
user_init_balance = ren_token.balanceOf(user)

Copy link
Member

Choose a reason for hiding this comment

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

this variable is not being used

# 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

ren_pool.cancelWithdrawRequest({'from': owner})

# Make sure the withdraw request does not exist anymore
assert ren_pool.withdrawRequests(owner) != amount
Copy link
Member

@fede-rodes fede-rodes Sep 26, 2021

Choose a reason for hiding this comment

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

I would say assert ren_pool.withdrawRequests(owner) == Null or something along those lines to reflect the non existence of the withdrawRequest. Well, actually, I'm not sure the request gets destroyed or is set back to zero (?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is set to zero I believe, I'll make some tests and rectify this

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants