BNBx integration #2

manoj9april · 2022-11-03T14:07:25Z

No description provided.

manoj9april · 2022-11-03T14:19:39Z

contracts/strategy/BnbxYieldConverterStrategy.sol

+    function claimFundsforUsers(){
+        // iterate over user requests 
+        // check if 15 days have passed after user has triggered requestWithdraw
+        // 


Unable to find a way to provide funds back to users

manoj9april · 2022-11-04T15:43:00Z

contracts/strategy/BnbxYieldConverterStrategy.sol

+
+        // uint256 batchId = 
+        _stakeManager.claimWithdraw(claimableIdx);
+        // _distributeFunds(batchId)


Shall we distribute rewards in FIFO order of user requests ?

galacticminter · 2022-11-08T00:44:20Z

contracts/strategy/BnbxYieldConverterStrategy.sol

+    /// @dev deposits the given amount of BNB into Stader stakeManager through bnbxRouter
+    function deposit() external payable onlyVault returns (uint256) {
+        uint256 amount = msg.value;
+        require(amount <= address(this).balance, "insufficient balance");


Do we need this check?

not required. added it as it was there in ceros samples.

galacticminter · 2022-11-08T00:45:03Z

contracts/strategy/BnbxYieldConverterStrategy.sol

+        return _deposit(amount);
+    }
+
+    /// @dev deposits all the available BNB into Stader stakeManager through bnbxRouter


...available BNB on router into stader ...

missed changing comments
should be
/// @dev deposits all the available BNB in this strategy contract to Stader stakeManager

Will check with Helio devs as well for its use case.
as it will deposit back the users funds. (unstaked funds)
Accordingly will have to update _bnbToDistribute

_bnbToDistribute = _bnbToDistribute - address(this).balance + msg.value ?

but there could be bnb transferred to this contract as well. as it contains a receive function

contracts/strategy/BnbxYieldConverterStrategy.sol

galacticminter · 2022-11-08T00:53:03Z

contracts/strategy/BnbxYieldConverterStrategy.sol

+        require(canDeposit(amount), "invalid amount");
+
+        _bnbDepositBalance += amount;
+        _bnbxHoldingBalance += _stakeManager.convertBnbToBnbX(amount);


@dulguun-staderlabs Should we instead calculate the difference of pre and post balances of BNBx than relying on the computed value?

@galacticminter Yes. Otherwise this contract is depending on the external contract output.

This is not relevant now. as we have removed the _bnbxHoldingBalance variable.

galacticminter · 2022-11-08T00:54:45Z

contracts/strategy/BnbxYieldConverterStrategy.sol

+        _bnbDepositBalance += amount;
+        _bnbxHoldingBalance += _stakeManager.convertBnbToBnbX(amount);
+        _stakeManager.deposit{value: amount}();
+        return amount;


If we are not planning to return BNBx to user or master strategy, can we add that to documentation? If we are, we have to transfer it to msg.sender, right?

sending BNBx to user is not a requirement. Discussed with Dheeraj.

galacticminter · 2022-11-08T00:57:35Z

contracts/strategy/BnbxYieldConverterStrategy.sol

+        internal
+        returns (uint256 value)
+    {
+        require(amount > 0, "invalid amount");


Why do we need this check over a uint256?

yes not required. solidity will automatically handle it.

galacticminter · 2022-11-08T01:27:14Z

contracts/strategy/BnbxYieldConverterStrategy.sol

+            if (isClaimable) {
+                foundClaimableReq = true;
+                claimableIdx = idx;
+                _bnbToDistribute += amount;


Why not move stakeManager.claimWithdraw(idx) here?

Return true instead of break.

Return false at the end of fn.

Can we return the Claim_amount - amount (as funds queued for undelegation will keep accruing rewards until actually unstaked?) to Stader treasury? Otherwise slowly leads to BNB buildup.

You mean,
after funds are sent back to this strategy contract, we will transfer it to stader treasury?

It would be better to handle in our contract (stakeManager)?

Nevermind this.

galacticminter · 2022-11-08T01:27:53Z

contracts/strategy/BnbxYieldConverterStrategy.sol

+        uint256 triggerTime;
+    }
+    mapping(uint256 => UserWithdrawRequest) private _withdrawRequests;
+    uint256 private _firstWithdrawIdx;


firstDistributeIdx

galacticminter · 2022-11-08T01:28:30Z

contracts/strategy/BnbxYieldConverterStrategy.sol

+    {
+        reqCount = 0;
+
+        while (


Replace with for.

contracts/strategy/BnbxYieldConverterStrategy.sol

galacticminter · 2022-11-08T01:32:46Z

contracts/strategy/BnbxYieldConverterStrategy.sol

+        for (uint256 idx = 0; idx < requests.length; idx++) {
+            (bool isClaimable, uint256 amount) = _stakeManager
+                .getUserRequestStatus(address(this), idx);
+            if (isClaimable) {


Make this structure less nested.

if (!claimable) continue; ...

galacticminter · 2022-11-08T01:33:58Z

contracts/strategy/BnbxYieldConverterStrategy.sol

+        external
+        returns (uint256 reqCount)
+    {
+        reqCount = 0;


Where are you incrementing reqCount?

oops, missed it.

contracts/strategy/BnbxYieldConverterStrategy.sol

galacticminter · 2022-11-08T01:39:51Z

contracts/strategy/BnbxYieldConverterStrategy.sol

+    /// @dev withdraws everything from Stader's stakeManager
+    function panic() external onlyStrategist returns (uint256) {
+        (, , uint256 debt) = _vault.strategyParams(address(this));
+        return _withdraw(address(_vault), debt);


How can we give away the power to withdraw any BNB to the strategist when all BNB incoming to this contract from the stakeManager has to be distributed to individual recipients?
Consider the following cases

Any BNB deposited immediately gets converted to BNBx.

Any BNB that gets transferred gets rejected (as there's no receive fn)

if debt is tracked by the master vault, why then are we tracking bnbDeposited?

Is the expectation that this panicked BNB will not be distributed to users later?

Does the master vault have bookkeeping to track this intervention?

Any BNB that gets transferred gets rejected (as there's no receive fn)

This contract contains a receive fn

if debt is tracked by the master vault, why then are we tracking bnbDeposited?

master has the ability to update debt i.e. debt = any number
updateDebt

Is the expectation that this panicked BNB will not be distributed to users later?

I don't think so. We can check with Helio folks while PR review.

Does the master vault have bookkeeping to track this intervention?

Couldn't find any bookkeeping for this. Will check with Helios folks during PR review again.
But they do have a waiting pool contract
which has ability to maually add withdraw entries and pay off users debts

pstake and Helio discussion over this.

As discussed with Dheeraj,
This is okay as it is not messing with bnbToDistribute or our book-keeping
Whereas it would just create a entry in our book-keeping for masterVault.

contracts/strategy/BnbxYieldConverterStrategy.sol

galacticminter · 2022-11-08T01:44:10Z

contracts/strategy/BnbxYieldConverterStrategy.sol

+    uint256 private _firstWithdrawIdx;
+    uint256 private _nextWithdrawIdx;
+
+    uint256 private _bnbxHoldingBalance; // amount of bnbx held by this strategy


Why do we need to keep track of this balance?
This is always bnbXtoken.balanceOf(address.of(this)) - bnbxToUnstake, isn't it?

We only track bnbxToUnstake and only bnbDeposited.

yes the above should work unless
someone transfers bnbx token to this contract ?

galacticminter · 2022-11-08T01:44:39Z

contracts/strategy/BnbxYieldConverterStrategy.sol

+        return _withdraw(recipient, amount);
+    }
+
+    /// @dev withdraws everything from Stader's stakeManager


Can you elaborate on everything

pstake and Helio discussion over this.
everything = bnb in strategy (users' unstaked funds + extraBNB if any) + bnb still deposited in stakeManager

And as per master vault's code, it is bnbDepositBalance i.e. bnb still deposited in stakeManager

Here everything will be bnbDepositBalance

…contracts into bnbx-integrate

manoj9april · 2022-11-08T14:22:39Z

contracts/strategy/BnbxYieldConverterStrategy.sol

+            triggerTime: block.timestamp
+        });
+
+        return 0;


I think we will have to return amount here. checked masterVault's code.
They are using this to update debt

galacticminter · 2022-11-09T15:33:27Z

contracts/strategy/BnbxYieldConverterStrategy.sol

+
+    /// @dev deposits all the available BNB into Stader stakeManager through bnbxRouter
+    function depositAll() external payable onlyVault returns (uint256) {
+        return _deposit(address(this).balance);


Should this be _deposit(address(this).balance - _bnbToDistribute);

manoj9april · 2022-11-10T09:30:59Z

contracts/strategy/BnbxYieldConverterStrategy.sol

+        return 0;
+    }
+
+    // actual withdraw request to stader


@galacticminter
Do we need to add any restriction on this function to be callable only after 24 hours?

This reverts commit 96cc305.

wip : bnbx-helio integration

1fdd74f

manoj9april commented Nov 3, 2022

View reviewed changes

wip: helio integration

e870aa7

manoj9april commented Nov 4, 2022

View reviewed changes

new design

7e5af61

manoj9april self-assigned this Nov 7, 2022

manoj9april added the enhancement New feature or request label Nov 7, 2022

manoj9april requested review from galacticminter and dulguun-staderlabs November 7, 2022 14:47

minor changes

895b683

galacticminter reviewed Nov 8, 2022

View reviewed changes

contracts/strategy/BnbxYieldConverterStrategy.sol Show resolved Hide resolved

galacticminter reviewed Nov 8, 2022

View reviewed changes

contracts/strategy/BnbxYieldConverterStrategy.sol Show resolved Hide resolved

galacticminter reviewed Nov 8, 2022

View reviewed changes

contracts/strategy/BnbxYieldConverterStrategy.sol Outdated Show resolved Hide resolved

galacticminter reviewed Nov 8, 2022

View reviewed changes

contracts/strategy/BnbxYieldConverterStrategy.sol Outdated Show resolved Hide resolved

galacticminter reviewed Nov 8, 2022

View reviewed changes

Merge branch 'add-masterVault' of github.com:stader-labs/helio-smart-…

2f892c5

…contracts into bnbx-integrate

manoj9april commented Nov 8, 2022

View reviewed changes

galacticminter reviewed Nov 9, 2022

View reviewed changes

Manoj Patra added 4 commits November 10, 2022 13:49

update with Base Strategy Design

614115e

variable rename

a772376

removed _bnbxHoldingBalance

9206b05

deposit flow improvements

218c990

manoj9april commented Nov 10, 2022

View reviewed changes

Manoj Patra added 10 commits November 10, 2022 16:44

withdraw flow improvement

e91df39

comments

961f37f

comments

3677322

added restriction on batchWithdraw

913b7a6

minor changes

577e8a9

add distribute within claim

96cc305

Revert "add distribute within claim"

9595377

This reverts commit 96cc305.

add claimAndDistribute

8e2c4f4

add manual distribute

54aa03a

add reentrancy guards

26dcbae

BNBx integration #2

Are you sure you want to change the base?

BNBx integration #2

Conversation

manoj9april commented Nov 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manoj9april Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

galacticminter Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manoj9april Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manoj9april Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

galacticminter Nov 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manoj9april Nov 8, 2022 •

edited

Loading

galacticminter Nov 8, 2022 •

edited

Loading

manoj9april Nov 8, 2022 •

edited

Loading

manoj9april Nov 8, 2022 •

edited

Loading

galacticminter Nov 9, 2022 •

edited

Loading