Bitcoin Redeemer implementation #335
Conversation
The contract is used to integrate stBTC unstaking with bridging to Bitcoin via tBTC Bridge. The `requestRedemption` function expects the owner of stBTC shares to approve stBTC token shares to the BticoinRedeemer contract. Then when `requestRedemption` function is called it redeems stBTC and calls TBTC token contract to redeem tBTC to Bitcoin.
The event is emitted after stbtc.redeem call, but we control the stBTC contract, so there is no risk.
We use the wrapper in tests instead the snapshots, so we want to be consistent.
For consistency with BitcoinRedeemerUpdated and DispatcherUpdated we added old treasury address in the event.
This function name should better describe what is happening inside.
Instead of requiring the user to approve and request redemption in two transactions we use approveAndCall pattern to cover it within one transaction.
✅ Deploy Preview for acre-dapp-testnet ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
We use @thesis-co/solidity-contracts in BitcoinRedeemer for receive approval interfaces.
The package is named: `@thesis-co/solidity-contracts` in its package.json file. To reduce confusion we match the name here.
Slither is reporting: ``` stBTC (contracts/stBTC.sol#24-365) should inherit from ITBTCToken (contracts/bridge/ITBTCToken.sol#7-25) Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#missing-inheritance ``` The stBTC contract shouldn't inherit from ITBTCToken contract.
We don't need the reference as it's not used in redemption process.
| function redeemSharesAndUnmint( | ||
| address owner, | ||
| uint256 shares, | ||
| bytes calldata tbtcRedemptionData |
There was a problem hiding this comment.
If the redeemer is set to the sBTC owner address, I believe this ~solves the problem of potential redemption timeouts. sBTC owner will be able to claim Bank balance in tBTC bridge. Not ideal UX but something.
There was a problem hiding this comment.
Do you think it should be enforced on-chain or it's enough for the SDK to prepare the redemption data correctly?
There was a problem hiding this comment.
I think it is enough to have it handled in the SDK but I would also make it very clear in the documentation of the redeemSharesAndUnmint function. This is absolutely not clear for someone who is not familiar with tBTC internals.
Current implementation of the redemptions mechanism assumes specific TBTCVault contract implementation to be the owner of the TBTC Token contract. We want to ensure that the owner of TBTC Vault haven't changed, so we don't accidentially lock the tBTC tokens.
|
Not directly related to |
| [-stBtcAmountToRedeem], | ||
| ) | ||
|
|
||
| expect(await stbtc.totalSupply()).to.be.equal( |
There was a problem hiding this comment.
Let's move it to a new it to check totalSupply.
| context("when tBTC.approveAndCall returns false", () => { | ||
| beforeAfterSnapshotWrapper() | ||
|
|
||
| before(async () => { |
There was a problem hiding this comment.
Thoughts about unpacking beforeAfteSnapshoWrapper and move tbtc.setApproveAndCallResult(false) to "before"? Now we have duplicates of "before".
There was a problem hiding this comment.
This is a pattern we use across the tests. We could improve it in the future globally for all tests in a separate PR.
Closes: #159
In this PR we integrate the withdrawal of tBTC from stBTC vault with tBTC Bridge redemption, so the user can withdraw directly to Bitcoin in one transaction.
For the Bitcoin-only experience, it is expected that the owner of the stake will be Safe with Bitcoin message signatures verification capability as per https://github.com/thesis/orangekit/tree/main/solidity.
This implementation is different from the initial implementation in #309 as we removed ERC20 permits to handle withdrawals, as dApp users will produce Bitcoin signatures for the Safe, not the EIP-712 Ethereum signatures.