Skip to content

Comments

Implement Gnosis withdrawals pull#45

Merged
dapplion merged 7 commits intomasterfrom
withdrawals-pull
Jul 11, 2023
Merged

Implement Gnosis withdrawals pull#45
dapplion merged 7 commits intomasterfrom
withdrawals-pull

Conversation

@dapplion
Copy link
Member

@dapplion dapplion commented Jun 7, 2023

Current master (via #25) implements push withdrawals. We originally decided to implement this strategy because:

  • Requires the least amount of actions from stakers, which get the withdraw tokens credited in their address by block processing
  • This behavior follows the UX in Ethereum, which is what stakers may expect here too

However @filoozom noted that since the ERC20 transfer happens as part of receipt-less system transaction, the Transfer logs are not accessible anywhere. This breaks downstream tooling and will cause token balances to be incorrectly reported. Otherwise tooling is forced to incorporate custom logic to handle this differences, raising the cost of the feature.

Due to this in the last core devs call consensus is formed that the incompatibility in tooling + the extra complexity does not justify going for the push strategy.

This PR implements a pull strategy, which requires a final claim step to credit the withdrawn tokens to stakers. The claim happens in a regular transaction after the withdrawal block, so the Transfer receipts are available. Also, the insolvency case does not need to be handled explicitly, which significantly reduces the complexity of the contract to basically this two functions:

function executeSystemWithdrawals(
uint256 _maxNumberOfFailedWithdrawalsToProcess,
uint64[] calldata _amounts,
address[] calldata _addresses
) external {
require(
_msgSender() == SYSTEM_WITHDRAWAL_EXECUTOR || _msgSender() == _admin(),
"This function should be called only by SYSTEM_WITHDRAWAL_EXECUTOR or _admin()"
);
assert(_amounts.length == _addresses.length);
for (uint256 i = 0; i < _amounts.length; ++i) {
// Divide stake amount by 32 (1 GNO for validating instead of the 32 ETH expected)
uint256 amount = (uint256(_amounts[i]) * 1 gwei) / 32;
withdrawableAmount[_addresses[i]] += amount;
}
}

function claimWithdrawal(address _address) external {
uint256 amount = withdrawableAmount[_address];
if (amount > 0) {
withdrawableAmount[_address] = 0;
stake_token.safeTransfer(_address, amount);
}
}

Note that anyone can claim for any address. I expect Gnosis devops or other interested parties (staking services) to sponsor and auto-claim on behalf of stakers such that the perceived UX for the staker is the same as with the push approach

@dapplion dapplion requested review from Barichek and filoozom June 7, 2023 18:23
@dapplion dapplion force-pushed the withdrawals-pull branch from 2e2eea9 to 961df1b Compare June 7, 2023 18:24
Barichek
Barichek previously approved these changes Jun 8, 2023
Copy link
Contributor

@Barichek Barichek left a comment

Choose a reason for hiding this comment

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

Seems good. I would just suggest to remove legacy comments above executeSystemWithdrawals function and remove uint256 _maxNumberOfFailedWithdrawalsToProcess from list of input parameters of it.

@dapplion
Copy link
Member Author

dapplion commented Jun 8, 2023

Seems good. I would just suggest to remove legacy comments above executeSystemWithdrawals function and remove uint256 _maxNumberOfFailedWithdrawalsToProcess from list of input parameters of it.

@Barichek we cannot change the function signature of executeSystemWithdrawals or the execution clients will break. What name would you suggest to assign the variable?

@filoozom filoozom mentioned this pull request Jun 19, 2023
@dapplion dapplion mentioned this pull request Jun 22, 2023
dapplion and others added 2 commits June 22, 2023 20:38
* chore: remove global imports

* chore: ignore _deprecatedUnused

* chore: re-add _deprecatedUnused comment
* chore: add system tx with right signature

* fix: make signature public and fix tests

* Update SBCDepositContract comment

* Update SBCDepositContract.sol

---------

Co-authored-by: Lion - dapplion <35266934+dapplion@users.noreply.github.com>
@dapplion dapplion merged commit 24f9fcf into master Jul 11, 2023
@dapplion dapplion deleted the withdrawals-pull branch July 11, 2023 09:30
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.

4 participants