feat: remove mGNO#31
Merged
dapplion merged 8 commits intognosischain:ib-withdrawal-contract-partfrom Mar 22, 2023
Merged
Conversation
Barichek
suggested changes
Mar 22, 2023
Contributor
Barichek
left a comment
Barichek
left a comment
There was a problem hiding this comment.
All the rest from the smart contract part seems good at first glance.
contracts/SBCDepositContract.sol
Outdated
| @@ -429,12 +396,7 @@ contract SBCDepositContract is | |||
|
|
|||
| for (uint256 i = 0; i < _amounts.length; ++i) { | |||
| uint256 amount = uint256(_amounts[i]) * 1 gwei; | |||
Contributor
There was a problem hiding this comment.
Here should be also / 32, as currently amount is multiplied by 32.
dapplion
added a commit
that referenced
this pull request
Mar 22, 2023
* withdrawal part first commit * finalized withdrawals * naming and lints * few more checks on withdrawals in deposit.ts tests * Additional reentrancy check on failed withdrawals + whenNotPaused for manual failed withdrawal processing * added gasLimit for withdrawals * call of processFailedWithdrawalsFromPointer in executeSystemWithdrawals * comments above IWithdrawalContract functions * Skip bad withdrawals * Only drop address 0 withdrawal * Test bad withdrawals * Add assertSuccessfulWithdrawal * Handle multiple event sin assertSuccessfulWithdrawal * Update test * feat: remove `mGNO` (#31) * wip: remove mGNO * test: fix remaining tests * chore: update error message * feat: add `unwrapTokens` function * wip: upgradeToAndCall * chore: remove package-lock and reset yarn.lock * Add division on withdraw --------- Co-authored-by: dapplion <35266934+dapplion@users.noreply.github.com> * Fix lint * Drop 'should upgrade and unwrap' test * Drop EIP1967UpgradeToAndCall * Update test EIP1967UpgradeToAndCall * Drop duplicated bad withdrawals tests for GNO / mGNO --------- Co-authored-by: dapplion <35266934+dapplion@users.noreply.github.com> Co-authored-by: Philippe Schommers <philippe@schommers.be>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR illustrates an example of how
mGONcould be removed altogether from the deposit and withdraw contracts.Reason
Gas efficiency
Unwrapping all the
mGNOtoGNOonce, instead of once per withdrawl, would make withdrawals more efficient. The same goes for deposits, as the wrapping step would be entirely removed.UX
For the user,
mGNOis just confusing. Why does it even need to exist in the first place? It feels like a hacky attempt to look like Ethereum, but in the end has no advantages except making integrations slightly easier. When users look at their balances, they want to know how muchGNOthey have, not how muchmGNO, so they can divide it by32to getGNO. When users deposit, it would also appear clearly in MetaMask that this is adepositcontract call, rather than atransferAndCallon some obscure token. This could actually even be seen as a security feature in that sense.Complexity
The
mGNOconcept makes all the contracts more complex, which makes them harder to audit and reason about. It's also code that impacts external tools like deposit UIs, and make their development more complicated.Key stakeholders
Beacon chain explorers
Beacon chain explorers would need to be updated in order to display
GNOinstead ofmGNO. For now, these are:Deposit UIs
Deposit UIs need to replace the
transferAndCalllogic with a simpledeposit/batchDepositcall on the deposit contract.Community
In general the community seems to prefer to using GNO as well: https://forum.gnosis.io/t/should-gnosis-chain-withdrawals-disperse-mgno-or-gno/6620
This PR
I have to refactor it, but at least we can get the conversation going here.