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

Expose deposit amounts in AbstractDepositorContract #791

Merged
merged 4 commits into from
Feb 28, 2024

Conversation

nkuba
Copy link
Member

@nkuba nkuba commented Feb 27, 2024

In this PR we introduce two changes to the AbstractDepositorContract around the deposit amounts.

Expose minimum deposit amount

Integrators may need a minimum deposit amount to introduce validation if the amount the user wants to deposit meets the Bridge requirement of a deposit to exceed the dust threshold.
Originally proposed in thesis/acre#253 (comment).

Return initial deposit amount from _initializeDeposit function

The amount may be useful for integrators for validations of the deposit amount. Originally proposed in thesis/acre#253 (comment).

It turns out that this version where we read from the storage:

        initialDepositAmount =
            bridge.deposits(depositKey).amount *
            SATOSHI_MULTIPLIER;

costs 117 366 gas.
Which is less than an alternative approach:

            initialDepositAmount =
                fundingTx
                    .outputVector
                    .extractOutputAtIndex(reveal.fundingOutputIndex)
                    .extractValue() *
                SATOSHI_MULTIPLIER;

which costs 117 601 gas.

We return initialDepositAmount from two functions: _initializeDeposit and _finalizeDeposit. I tried to introduce a getter function:

    function _getInitialDepositAmount(
        uint256 depositKey
    ) internal view returns (uint256) {
        IBridgeTypes.DepositRequest memory deposit = bridge.deposits(
            depositKey
        );
        require(deposit.revealedAt != 0, "Deposit not initialized");

        return deposit.amount * SATOSHI_MULTIPLIER;
    }

and removed initialDepositAmount return from _initializeDeposit and _finalizeDeposit functions.
Unfortunately, the overall cost in the reference BitcoinDepositor implementation from thesis/acre#253 wasn't improved:
BEFORE:

+ initializeStake --> 143 004
+ finalizeStake --> 195 178
= total --> 338 182

AFTER:

+ initializeStake --> 142 912
+ finalizeStake --> 197 960
= total --> 340 872

Integrators may need minimum deposit amount to introduce validation if
the amount user wants to deposit meets Bridge requirement of a deposit
to exceed the dust threshold.
Copy link

Solidity API documentation preview available in the artifacts of the https://github.com/keep-network/tbtc-v2/actions/runs/8063943343 check.

The amount may be useful for integrators for validations of the deposit
amount. See example use case in thesis/acre#253 (comment)

It turns out that this version where we read from the storage costs
`117 366` gas. Which is less than an alternative approach:
```
        initialDepositAmount =
            fundingTx
                .outputVector
                .extractOutputAtIndex(reveal.fundingOutputIndex)
                .extractValue() *
            SATOSHI_MULTIPLIER;
```
which costs `117 601` gas.
Copy link

Solidity API documentation preview available in the artifacts of the https://github.com/keep-network/tbtc-v2/actions/runs/8064155396 check.

@nkuba nkuba changed the title Expose minimum deposit amount in AbstractDepositorContract Expose deposit amounts in AbstractDepositorContract Feb 27, 2024
@nkuba nkuba marked this pull request as ready for review February 27, 2024 12:03
nkuba added a commit to thesis/acre that referenced this pull request Feb 27, 2024
The abstract contract exposes the values, so the implementation doesn't
have to interact with the bridge and do any precision conversion.
These changes depend on:
keep-network/tbtc-v2#791
Copy link
Member

@lukasz-zimnoch lukasz-zimnoch left a comment

Choose a reason for hiding this comment

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

Regarding _getInitialDepositAmount, did you try to use the storage pointer:

IBridgeTypes.DepositRequest storage deposit = bridge.deposits(
 depositKey
);

instead of the presented:

IBridgeTypes.DepositRequest memory deposit = bridge.deposits(
 depositKey
);

?

The storage pointer requires 2 storage reads while the version with memory reads all 7 storage fields of the DepositRequest. This can explain the increased gas consumption. The current version works for me but if we prefer a separate function, we can give it a shot.

@nkuba
Copy link
Member Author

nkuba commented Feb 28, 2024

Regarding _getInitialDepositAmount, did you try to use the storage pointer:

IBridgeTypes.DepositRequest storage deposit = bridge.deposits(
 depositKey
);

instead of the presented:

IBridgeTypes.DepositRequest memory deposit = bridge.deposits(
 depositKey
);

?

The storage pointer requires 2 storage reads while the version with memory reads all 7 storage fields of the DepositRequest. This can explain the increased gas consumption. The current version works for me but if we prefer a separate function, we can give it a shot.

Unfortunately, I cannot do this as bridge.deposits returns memory variable:

TypeError: Type struct IBridgeTypes.DepositRequest memory is not implicitly convertible to expected type struct IBridgeTypes.DepositRequest storage pointer.
   --> @keep-network/tbtc-v2/contracts/integrator/AbstractTBTCDepositor.sol:295:9:
    |
295 |         IBridgeTypes.DepositRequest storage deposit = bridge.deposits(
    |         ^ (Relevant source part starts here and spans across multiple lines).


Error HH600: Compilation failed

Do you see another way to get through it?

_initializeDeposit now returns additional property:
initialDepositAmount.
We need to update the usage example.
@nkuba nkuba self-assigned this Feb 28, 2024
Copy link

Solidity API documentation preview available in the artifacts of the https://github.com/keep-network/tbtc-v2/actions/runs/8079203442 check.

@lukasz-zimnoch
Copy link
Member

Unfortunately, I cannot do this as bridge.deposits returns memory variable:

Oh yeah, that's a call to the Bridge so an alternative would be exposing a dedicated function on the Bridge's side. Not worth bothering about it in that case.

@lukasz-zimnoch lukasz-zimnoch merged commit 9e047d1 into main Feb 28, 2024
38 checks passed
@lukasz-zimnoch lukasz-zimnoch deleted the abstract-depositor-deposit-amount branch February 28, 2024 10:59
nkuba added a commit to thesis/acre that referenced this pull request Feb 28, 2024
The newest version contains changes from keep-network/tbtc-v2#791
dimpar added a commit to thesis/acre that referenced this pull request Mar 6, 2024
This PR enhances #91.
Depends on keep-network/tbtc-v2#787
Depends on keep-network/tbtc-v2#791

### Introduction

In this PR we introduce a mechanism for the dApp to throttle stake
request initialization.

The staking flow for Bitcoin is asynchronous, consisting of a couple of
stages between the user entering the staking form in the dApp and the
tBTC being deposited in the stBTC vault.

Since the stBTC vault introduced a limit for maximum total assets under
management, there is a risk that the user will initialize staking that
won't be able to finalize in stBTC, due to concurrent other users'
stakes. We need to reduce such risk by throttling stake initialization
flow in the dApp.

### Soft Cap and Hard Cap Limits

#### Hard Cap

stBTC contract defines a _maximum total assets limit_, that cannot be
exceeded with new tBTC deposits. This is considered a hard limit, that
when reached no new deposits can be made, and stake requests in the
Bitcoin Depositor contract will be queued. These queued requests will
have to wait until the limit is raised or other users withdraw their
funds, making room for new users.

#### Soft Cap

Bitcoin Depositor Contract defines a _maximum total assets soft limit_,
which is assumed to be much lower than the _hard cap_. The limit is used
to throttle stakes and use a difference between _hard cap_ and _soft
cap_ as a buffer for possible async stakes coming from the dApp.

### Stake Amount Limits

#### Minimum Stake Limit

The Bitcoin Depositor contract defines a _minimum stake limit_, that is
used to define a minimum amount of a stake. The limit has to be higher
than the _deposit dust threshold_ defined in the tBTC Bridge, which is
validated on the deposit reveal.
The _minimum stake limit_ has to take into account the _minimum deposit
limit_ defined in the stBTC contract, and consider that the amount of
tBTC deposited in the stBTC vault on stake finalization will be reduced
by tBTC Bridge fees and Depositor fee.

#### Maximum Stake Limit

The Bitcoin Depositor contract defines a _maximum stake limit_, which is
the maximum amount of a single stake request. This limit is used to
introduce granularity to the stake amount and reduce the possibility of
a big stake request using the whole soft limit shared across other
concurrent stake requests.

### Usage in dApp

The limits should be validated in the dApp staking flow to reduce the
possibility of user stakes being stuck in the queue.

The contract exposes two functions to be used in the dApp
`minStakeInSatoshi` and `maxStakeInSatoshi`, for convenience the result
is returned in the satoshi precision.

### Flow


![image](https://github.com/thesis/acre/assets/10741774/46f699d1-3607-4e27-b07a-de18b6078fbd)
[source:
Figjam](https://www.figma.com/file/5S8Wa5WudTQGbKMk7ETKq8/Bitcoin-Depositor-Staking-Limits?type=whiteboard&node-id=1-519&t=aCatffRBQpe4BCMn-4)
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.

2 participants