Skip to content
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

Fixing withdrawal functionality in stBTC #350

Merged
merged 8 commits into from
Apr 23, 2024
Merged

Fixing withdrawal functionality in stBTC #350

merged 8 commits into from
Apr 23, 2024

Conversation

dimpar
Copy link
Member

@dimpar dimpar commented Apr 12, 2024

Upon withdrawal we need to take into account fees that will be collected by the treasury. A user will get the desired assets, but on top of that we need to pull a bit more funds from Mezo covering withdrawal fees.

Added tests to cover scenarios with allocated funds to Mezo Portal and then withdrawing and redeeming from Acre.

Upon withdrawal we need to take into account fees that will be collected
by the treasury. A user will get the desired assets, but on top we need
to pull the funds from Mezo covering not only what a user wants but also
pull a bit more to cover the withdrawal fees.

Added tests to cover scenarios with allocated funds to Mezo Portal and
then withdrawing and redeeming from Acre.
@dimpar dimpar self-assigned this Apr 12, 2024
Copy link

netlify bot commented Apr 12, 2024

Deploy Preview for acre-dapp-testnet canceled.

Name Link
🔨 Latest commit 900f487
🔍 Latest deploy log https://app.netlify.com/sites/acre-dapp-testnet/deploys/66279dbdf1fafc00082fc23c

@dimpar dimpar requested a review from nkuba April 12, 2024 11:15
@dimpar dimpar added the 🔗 Solidity Solidity contracts label Apr 12, 2024
dimpar added 2 commits April 15, 2024 14:31
We need to calculate fees that are associated with withdrawal assets
upon withdrawing when calculating how much assets should be withdarawn
from the dispatcher contract. Instead of calling a preview and
conversion functions we can modify access modifiers of _feeOnRaw and
_feeOnTotal to internal and use it in stBTC. That would simplify
calculations.
solidity/contracts/stBTC.sol Outdated Show resolved Hide resolved
solidity/test/stBTC.test.ts Show resolved Hide resolved
Comment on lines +911 to +912
const expectedRedeemedAssets =
(await stbtc.previewRedeem(to1e18(2))) - 2n
Copy link
Member

Choose a reason for hiding this comment

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

Is there a chance we could hardcode the expected value and add comments clarifying calculations?

Copy link
Member Author

@dimpar dimpar Apr 20, 2024

Choose a reason for hiding this comment

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

I replaced most of the preview* functions with the helpers feeOnTotal and feeOnRaw functions. Note that feeOnTotal and feeOnRaw are tested as part of this file. We have a lot to cover and later to maintain. Calculating everything by hand adds more work, especially if we want to change the fees. If for any reason we decide to simulate different fee other than what we have now, all the tests will fail that touch fees and will need to be recalculated again. Internal helper function feeOnTotal and feeOnRaw replace the manual calculation which will be done exactly same way and makes the development faster and easier. Besides we have a couple of tests with the manual calculations too, I think we can rely on helper functions (that are tested) and make the development easier.

solidity/test/stBTC.test.ts Show resolved Hide resolved
solidity/test/stBTC.test.ts Show resolved Hide resolved
solidity/test/stBTC.test.ts Show resolved Hide resolved
@dimpar dimpar mentioned this pull request Apr 22, 2024
@nkuba nkuba enabled auto-merge April 23, 2024 11:38
@nkuba nkuba merged commit 9902f4c into main Apr 23, 2024
24 checks passed
@nkuba nkuba deleted the mo-tests branch April 23, 2024 11:41
nkuba added a commit that referenced this pull request Apr 24, 2024
Depends on #350

Covering `totalAssets()` with tests.
nkuba added a commit that referenced this pull request Apr 25, 2024
Depends on #350

This configuration is aimed to run `MezoAllocator` tests against
`mainnet` fork and check the flow with the real [Mezo Portal
contract](https://etherscan.io/address/0xAB13B8eecf5AA2460841d75da5d5D861fD5B8A39#code)
deployed on mainnet. This also sets up a ground for other tests that can
also be moved and checked against the real contracts on mainnet.

The network is called "integration". A local hardhat node should be run
to execute these tests.
E.g.
```
 npx hardhat node --no-deploy --fork https://eth-mainnet.g.alchemy.com/v2/<key> --fork-block-number 19680873
```
`19680873` the current block number was used as of writing these tests. 

Once the local node is running, then the tests can be executed by
running:
```
 pnpm test:integration
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔗 Solidity Solidity contracts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants