AIP-1 bonding #170

kupermind · 2024-07-14T13:06:20Z

Initial implementation and testing of dynamic IDF and bonding refactor.

…rrecting tests

kupermind · 2024-07-14T13:07:25Z

contracts/Depository.sol

+    // Current OLAS payout
+    // This value is bound by the initial total supply
+    uint96 payout;


Adding as part of dynamic IDF requirement

kupermind · 2024-07-14T13:07:42Z

contracts/Depository.sol

-    // Account address
-    address account;


Account is not needed as bonds become ERC-721 tokens

kupermind · 2024-07-14T13:12:15Z

contracts/Depository.sol

    /// #if_succeeds {:msg "payout"} old(mapBondProducts[productId].supply) == mapBondProducts[productId].supply + payout;
    /// #if_succeeds {:msg "OLAS balances"} IToken(mapBondProducts[productId].token).balanceOf(treasury) == old(IToken(mapBondProducts[productId].token).balanceOf(treasury)) + tokenAmount;
-    function deposit(uint256 productId, uint256 tokenAmount) external
+    function deposit(uint256 productId, uint256 tokenAmount, uint256 bondVestingTime) external


As part of the AIP-1 for dynamic IDF, the bonder can choose the bond vesting time, just not to be below the minimum allowed one.

77ph · 2024-07-24T10:46:01Z

contracts/BondCalculator.sol

+        // Check the sum of factors that cannot exceed the value of 10_000 (100% with a 0.01% step)
+        uint256 sumWeights;
+        for (uint256 i = 0; i < _discountParams.weightFactors.length; ++i) {
+            sumWeights += _discountParams.weightFactors[i];


_discountParams.weightFactors[i] >0 and sumWeights > 0 ? or some ki can be zero?

weights can all be zero, leaving as is.

77ph · 2024-07-24T10:57:15Z

contracts/GenericBondCalculator.sol

+    function calculatePayoutOLAS(
+        uint256 tokenAmount,
+        uint256 priceLP,
+        bytes memory data


I'm not sure that we should use implicit pack of arguments as a raw sequence of bytes (memory data) and then unpack it (abi.decode) instead of explicitly enumerating the arguments.

77ph · 2024-07-24T11:08:54Z

contracts/Depository.sol

-        payout = IGenericBondCalculator(bondCalculator).calculatePayoutOLAS(tokenAmount, product.priceLP);
+        payout = IBondCalculator(bondCalculator).calculatePayoutOLAS(tokenAmount, product.priceLP,
+            // Encode parameters required for the IDF calculation
+            abi.encode(msg.sender, bondVestingTime, productMaxVestingTime, supply, product.payout));


abi.encode(msg.sender, bondVestingTime, productMaxVestingTime, supply, product.payout)
This design seems like overengineering to me.
better just
IBondCalculator(bondCalculator).calculatePayoutOLAS(tokenAmount, product.priceLP,
// Encode parameters required for the IDF calculation
msg.sender, bondVestingTime, productMaxVestingTime, supply, product.payout);
In case of any errors, this will make debugging much more difficult.

77ph · 2024-07-24T11:17:59Z

contracts/Depository.sol

+        // Create and mint a new bond
+        bondId = totalSupply;
+        // Safe mint is needed since contracts can create bonds as well
+        _safeMint(msg.sender, bondId);


audit notes: re-check reentrancy in internal audit

re-check, please, in https://github.com/valory-xyz/autonolas-registries/blob/main/contracts/ServiceRegistryL2.sol: logic: serviceId = totalSupply; serviceId++; totalSupply = serviceId; // Mint the service instance to the service owner and record the service structure _safeMint(serviceOwner, serviceId);

so, id++ then _safeMint(serviceOwner, serviceId);
here:
bondId = totalSupply;
// Safe mint is needed since contracts can create bonds as well
_safeMint(msg.sender, bondId);
totalSupply = bondId + 1;

service Ids indexing must start from zero due to possible mapping conflicts. There is no such thing for bonds.

kupermind · 2024-07-31T16:46:10Z

contracts/BondCalculator.sol

+    // Maximum sum of discount factor weights
+    uint256 public constant MAX_SUM_WEIGHTS = 10_000;


Sum of weights for k fractions - using the Vote Weighting terminology let them be from 0 to 100% with a 0.01% step.

kupermind · 2024-07-31T16:47:10Z

contracts/BondCalculator.sol

+struct DiscountParams {
+    // DAO set voting power limit for the bonding account
+    // This value is bound by the veOLAS total voting power
+    uint96 targetVotingPower;
+    // DAO set number of new units per epoch limit
+    // This number is bound by the total number of possible components and agents
+    uint64 targetNewUnits;
+    // DAO set weight factors
+    // The sum of factors cannot exceed the value of 10_000 (100% with a 0.01% step)
+    uint16[4] weightFactors;
+}


Corresponds to 2 variables and 4 weights set by the DAO.

kupermind · 2024-07-31T17:18:36Z

contracts/BondCalculator.sol

+    /// @param productSupply Current product supply.
+    /// @param productPayout Current product payout.
+    /// @return amountOLAS Resulting amount of OLAS tokens.
+    function calculatePayoutOLAS(


This is left as is from the GenericBondCalculator, which is now obsolete.

kupermind · 2024-07-31T17:19:40Z

contracts/BondCalculator.sol

+
+        // Amount with the discount factor is IDF * priceLP * tokenAmount / 1e36
+        // At this point of time IDF is bound by the max of uint64, and totalTokenValue is no bigger than the max of uint192
+        amountOLAS = (idf * totalTokenValue) / 1e36;


This is the only new difference - tokenomics-calculated IDF is changed with the local calculated dynamic IDF according to the proposed approach.

kupermind · 2024-07-31T17:21:32Z

contracts/Depository.sol

+        uint256 productMaxVestingTime = product.vesting;
+        // Calculate vesting limits
+        if (bondVestingTime < MIN_VESTING) {
+            revert LowerThan(bondVestingTime, MIN_VESTING);
+        }
+        if (bondVestingTime > productMaxVestingTime) {
+            revert Overflow(bondVestingTime, productMaxVestingTime);
+        }


Bond vesting time is now chosen by the user from MIN_VESTING to max product defined vesting time.

kupermind · 2024-07-31T17:22:09Z

contracts/Depository.sol

        // Note that payout cannot be zero since the price LP is non-zero, otherwise the product would not be created
-        payout = IGenericBondCalculator(bondCalculator).calculatePayoutOLAS(tokenAmount, product.priceLP);
+        payout = IBondCalculator(bondCalculator).calculatePayoutOLAS(msg.sender, tokenAmount, product.priceLP,


Calls new BondCalculator contract based on proposed dynamic IDF.

What's the dynamic IDF again - how does it differ from previous one?

Dynamic IDF based on 4 parameters described in the doc: number of new units per epoch, vesting time (bound by the product vesting time), bond liquidity coefficient (the more one buys from the beginning of the product, the bigger IDF they get), veOLAS amount of the bonder.

In the previous approach the IDF was a pure tokenomics value based on the number of new units added in the epoch, and new owners providing units during the epoch as well.

kupermind · 2024-07-31T17:22:49Z

contracts/Depository.sol

+        // Adjust payout and set supply to zero if supply drops below the min defined value
+        if (supply < minOLASLeftoverAmount) {
+            payout += supply;
+            supply = 0;
+        }


If the product supply drops below a specified value, it's remainder is given to the bonder, and the product is instantly closed.

kupermind · 2024-07-31T17:24:06Z

contracts/Depository.sol

 }

 /// @title Bond Depository - Smart contract for OLAS Bond Depository
 /// @author AL
 /// @author Aleksandr Kuperman - <[email protected]>
-contract Depository is IErrorsTokenomics {
+contract Depository is ERC721, IErrorsTokenomics {


Bonds become NFT-s, yay!

DavidMinarsch · 2024-08-03T01:33:05Z

contracts/BondCalculator.sol

+    // DAO set voting power limit for the bonding account
+    // This value is bound by the veOLAS total voting power


Description is not very clear. what does the value affect?

Added description in #179

DavidMinarsch · 2024-08-03T01:33:19Z

contracts/BondCalculator.sol

+    // DAO set voting power limit for the bonding account
+    // This value is bound by the veOLAS total voting power
+    uint96 targetVotingPower;
+    // DAO set number of new units per epoch limit


New units for what?

Added a description in #179

DavidMinarsch · 2024-08-03T01:34:35Z

contracts/GenericBondCalculator.sol

@@ -1,93 +0,0 @@
-// SPDX-License-Identifier: MIT


Why is all this logic no longer needed?

Generic bond calculator used the old IDF approach, that is no longer the case. The new approach requires a new set of functions.

DavidMinarsch · 2024-08-03T01:35:12Z

contracts/Tokenomics.sol

-        // Check the epsilonRate value for idf to fit in its size
-        // 2^64 - 1 < 18.5e18, idf is equal at most 1 + epsilonRate < 18e18, which fits in the variable size
-        // epsilonRate is the part of the IDF calculation and thus its change will be accounted for in the next epoch
-        if (_epsilonRate > 0 && _epsilonRate <= 17e18) {
-            epsilonRate = uint64(_epsilonRate);
-        } else {
-            _epsilonRate = epsilonRate;
-        }


why was this dropped?

epsilonRate was used to set the default addition to the discount factor set by the DAO. In a new IDF approach, its value varies from 1 to 2 only (whereas in the old approach it could go up to 17), and thus no defaults are needed.

DavidMinarsch · 2024-08-03T01:35:32Z

contracts/Tokenomics.sol

-                        // Check if the owner has introduced component / agent for the first time
-                        // This is done together with the new unit check, otherwise it could be just a new unit owner
-                        address unitOwner = IToken(registries[unitType]).ownerOf(serviceUnitIds[j]);
-                        if (!mapNewOwners[unitOwner]) {
-                            mapNewOwners[unitOwner] = true;
-                            mapEpochTokenomics[curEpoch].epochPoint.numNewOwners++;
-                        }


why removed?

This is no longer used anywhere in the calculations.

DavidMinarsch · 2024-08-03T01:39:47Z

contracts/Tokenomics.sol

@@ -1,7 +1,6 @@
 // SPDX-License-Identifier: MIT
 pragma solidity ^0.8.25;


Not sure we should change Tokenomics at all. Feels like we could get away with keeping the existing implementation ...

Dev incentives revision force us to change tokenomics anyway.

The idea of calculating dev incentives in a new way is to calculate top-ups based on donator veOLAS amount. Right now top-ups are calculated based on the overall ETH amount donated throughout the epoch, and used as a factor in calculating top-ups (i.e., 10 ETH donated, 20 OLAS dev inflation - we split 20 OLAS the same way as 10ETH are split, considering the donator had enough veOLAS).

In a proposed approach veOLAS amount is a major variable for top-ups distribution - amount of veOLAS is split between components / agents donated. ETH is no longer in the equation of top-ups.

I see. However, ETH is still being used as part of the donation and distributed to all components/agents. Is it distributed according to the same proportion as the boosts?

The ETH distribution is going to be left as is. Meaning that proportions will be different.

DavidMinarsch · 2024-08-03T01:44:31Z

contracts/Depository.sol

        olas = _olas;
        tokenomics = _tokenomics;
        treasury = _treasury;
        bondCalculator = _bondCalculator;
+        baseURI = _baseURI;


Doesn't make sense here imo. We can have a purely on-chain NFT. There are these SVG libraries that are pretty cool. We don't want to have to serve custom images here, just unnecessary.

Actually we don't need any link at all. All we need to comply with the standard is that the function tokenURI() does not revert. So it can be as simple as just returning an NFT id as a string.

It's a bit messy to not offer an image. Will come back

DavidMinarsch · 2024-08-03T01:46:10Z

contracts/Depository.sol

        // Note that payout cannot be zero since the price LP is non-zero, otherwise the product would not be created
-        payout = IGenericBondCalculator(bondCalculator).calculatePayoutOLAS(tokenAmount, product.priceLP);
+        payout = IBondCalculator(bondCalculator).calculatePayoutOLAS(msg.sender, tokenAmount, product.priceLP,


What's the dynamic IDF again - how does it differ from previous one?

DavidMinarsch · 2024-08-03T01:48:56Z

contracts/BondCalculator.sol

+
+    /// @dev Changed inverse discount factor parameters.
+    /// @param newDiscountParams Struct of new discount parameters.
+    function changeDiscountParams(DiscountParams memory newDiscountParams) external {


So this is global, not product specific?

Yes, as otherwise this would need to be voted on for every product.

Technically each product is voted on. So wouldn't it be more useful to be able to set this as part of the product? Just thinking out loud.

Considering how products are launched, those would be pretty much the same. Also, if something happens on a protocol level, my understanding is that we want all the bonds to follow new parameters even for unfinished products, since otherwise the old params could become wrong / unjustified.

DavidMinarsch · 2024-08-03T01:49:38Z

contracts/BondCalculator.sol

+    /// @param productSupply Current product supply.
+    /// @param productPayout Current product payout.
+    /// @return amountOLAS Resulting amount of OLAS tokens.
+    function calculatePayoutOLAS(


kupermind added 7 commits July 10, 2024 18:32

refactor: initial additions of aip-1 specs

a0737ee

feat: enhancing dynamic idf calculation

e092d4e

feat: enhancing dynamic idf calculation

8280452

feat: enhancing dynamic idf calculation

dc5a551

refactor: make bonds ERC721 tokens

2ef7e3a

refactor: simplifying bond checks

0c254b1

refactor and test: making bond calculator inherit the generic one, co…

21bf1d9

…rrecting tests

kupermind requested review from DavidMinarsch and 77ph July 14, 2024 13:06

kupermind commented Jul 14, 2024

View reviewed changes

kupermind added 9 commits July 14, 2024 14:13

chore: adding a comment

002218c

test: adding bond calculator tests

721703f

chore: linter

1493900

chore: linter

f921c5c

test: fixing tests

543d825

test: forge

0868302

test: forge

8311624

chore: remove unused import

dc18a3d

refactor: remove numNewOwners tracking

bb35f9a

77ph reviewed Jul 24, 2024

View reviewed changes

77ph previously approved these changes Jul 24, 2024

View reviewed changes

refactor: accounting for PR comments

7035d04

kupermind dismissed 77ph’s stale review via 7035d04 July 24, 2024 13:41

kupermind commented Jul 31, 2024

View reviewed changes

DavidMinarsch reviewed Aug 3, 2024

View reviewed changes

		// Maximum sum of discount factor weights
		uint256 public constant MAX_SUM_WEIGHTS = 10_000;

		// DAO set voting power limit for the bonding account
		// This value is bound by the veOLAS total voting power

		@@ -1,7 +1,6 @@
		// SPDX-License-Identifier: MIT
		pragma solidity ^0.8.25;

AIP-1 bonding #170

Are you sure you want to change the base?

AIP-1 bonding #170

Conversation

kupermind commented Jul 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

77ph Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

77ph Jul 24, 2024 • 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

kupermind Aug 5, 2024 • 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

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

77ph Jul 24, 2024 •

edited

Loading

77ph Jul 24, 2024 •

edited

Loading

kupermind Aug 5, 2024 •

edited

Loading