-
Notifications
You must be signed in to change notification settings - Fork 4
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
WIP: Contract refactor #10
base: main
Are you sure you want to change the base?
Conversation
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.
Awesome. This is definitely the direction we want to go!
I added a few comments - curious what you think
event Deposit(address committer, uint256 amount); | ||
event Withdrawal(address committer, uint256 amount); | ||
event Deposit(address committer, uint256 amount, uint256 committerBalance); | ||
event FundsSlashed(address committer, uint256 amount, uint256 slashedBalance); |
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.
do we need a separate funds slashed event in addition to CommittmentEnded (which includes the amountPenalized)?
or maybe this is here for debugging purposes?
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.
There's certainly room for polishing the events. For instance, emitting events also consumes gas and in some cases there are now at least 2 events. So we might only want to do them on the public level, so for owner/consumer functions.
Maybe something like CommitmentEnded(committer, success, amount)?
require(_settleCommitment(commitment), "SPC::processCommitmentUser - settlement failed"); | ||
|
||
emit CommitmentEnded(committer, commitment.met, commitment.stake); | ||
function processAndSettle(address commitmentOwner) public returns (bool success){ |
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.
in various places we refer to the address that has made a committment as either committer
or committmentOwner
. We should be consistent in how we name this. I have a slight preference for the former, but don't really care as long as we're consistent throughout.
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.
Good point, I'll harmonize them
// @notice Public function to withdraw unstaked balance of user | ||
// @param amount Amount of <token> to withdraw | ||
/// @dev Check balances and active stake, withdraw from balances, emit event | ||
function _withdraw(uint256 amount, address committer) 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.
Since this function is only called by _settleCommitment
, I wonder if it makes sense to get rid of the require checks and other preliminary logic. Since we're only withdrawing the returned stake from a specific commitment, technically we don't need to check the global balance for the committer, which means we can also get rid of the available calculations.
I'm thinking we can do something like this...
function _withdraw(uint256 amount, address committer) internal {
_changeCommitterBalance(committer, amount, false);
require(token.transfer(committer, amount), "SPC::withdraw - token transfer failed");
emit Withdrawal(committer, amount, committerBalances[committer]);
}
The other option here is to forgo using a separate function altogether and embed those three lines into _settleCommitment
directly. Once we enable multiple commitments per user, we would bring back the available balance checks.
Or, since on Matic gas is cheap af maybe we don't need to worry about this at all until we establish a more permament architecture...thoughts?
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.
My general thought is that calling internal functions is free, and when omitting the events the functions are quite clear. But also a little to much for the scope that we have now :) I'll do some updates this afternoon. If the function stays easy to read I'm in also in favor of mergin settle, withdraw & slash.
But with this flow, for the user, it is not possible to withdraw their funds at their own volition.. Unless we say you deposit what you stake and you withdraw what you stake after success. I think that could fit/work for the SinglePlayer mode, but if we're going to think about a generic contract with another contract that is the SPC implementation, that should change.
uint256 available = committerBalances[committer]; | ||
Commitment storage commitment = commitments[committer]; | ||
|
||
if(commitment.exists == true){ |
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.
this can be simply if(commitment.exists)
MODIFIERS | ||
********/ | ||
modifier noCommitment { | ||
require(commitments[msg.sender].exists == false, "SPC::noCommitment - msg.sender has an active commitment"); |
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.
this can be !commitments[msg.sender].exists
} | ||
|
||
modifier hasCommitment { | ||
require(commitments[msg.sender].exists == true, "SPC::hasCommitment - msg.sender has no active commitment"); |
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.
this can be commitments[msg.sender].exists
} | ||
|
||
modifier activeCommitment { | ||
require((commitments[msg.sender].exists == true && commitments[msg.sender].met == false), "SPC::hasCommitment - msg.sender has no active commitment"); |
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.
this can be commitments[msg.sender].exists &&
etc.
TODO: