Sc 828 address omega review comments by FHieser · Pull Request #680 · InverterNetwork/contracts

FHieser · 2024-10-24T11:26:35Z

These comments are still unadressed here:
First @rororowyourboat

The tokenvault cant be set during init because is created simultaneously to the BondingCurve contract. It has to be added via Dependency Injection

linear · 2024-10-24T11:26:37Z

SC-828 Address Omega review comments

Zitzak · 2024-10-25T10:47:52Z

src/modules/fundingManager/bondingCurve/FM_BC_BondingSurface_Redeeming_v1.sol

    /// @dev Minimum collateral reserve
-    uint public constant MIN_RESERVE = 1 ether;
+    uint public MIN_RESERVE;


Curious if it's still considered a constant as it is being set during initialization? 🤔

Yeah thought about that too
Its just in the technicality a constant, at that It wont change anymore after we set it
I think I would keep it as its technically immutable, because we cant modify it without changing the code

Zitzak · 2024-10-25T10:48:44Z

src/modules/fundingManager/bondingCurve/FM_BC_BondingSurface_Redeeming_v1.sol

        // Set accepted token
        _token = IERC20(_acceptedToken);

+        // MIN_RESERVE is in dependency to the decimals of the workflow token


Suggested change

// MIN_RESERVE is in dependency to the decimals of the workflow token

// MIN_RESERVE is relational to the decimals of the workflow/colalteral token

Zitzak · 2024-10-25T10:52:39Z

src/modules/fundingManager/bondingCurve/FM_BC_BondingSurface_Redeeming_v1.sol

+
        token().safeTransfer(to, amount);

+        if (MIN_RESERVE > token().balanceOf(address(this))) {


We could also add the check for MIN_RESERVE into the if statement above, i.e. if (amount > token().balanceOf(address(this)) - projectCollateralFeeCollected + MIN_RESERVE) revert...

We decided to keep it here, as it creates more information in case of fail state
In case of the _Redeeming_Restricted_Repayer_Seizable_v1.sol we will adapt it though

Zitzak · 2024-10-25T10:53:45Z

...undingManager/bondingCurve/FM_BC_BondingSurface_Redeeming_Restricted_Repayer_Seizable_v1.sol

        // Set accepted token
        _token = IERC20(_acceptedToken);

+        // MIN_RESERVE is in dependency to the decimals of the workflow token


Suggested change

// MIN_RESERVE is in dependency to the decimals of the workflow token

// MIN_RESERVE is relational to the decimals of the workflow/colalteral token

Zitzak · 2024-10-25T10:55:59Z

...undingManager/bondingCurve/FM_BC_BondingSurface_Redeeming_Restricted_Repayer_Seizable_v1.sol

        __Module_orchestrator.fundingManager().token().safeTransfer(
            _to, _amount
        );
+        if (MIN_RESERVE > token().balanceOf(address(this))) {


See comment in FM_BC_BondingSurface_Redeeming_v1

Now that I looked into it again I decided against implementing it here, as the _getRepayableAmount doesnt directly interact with the actual amount of tokens that are still in the contract.
I think the way I implemented it now makes more sense, so I would keep it that way

…review-comments

@dev

* [SC-213] Feat: Reserve Vault (#665) * Create Reservepool Contract * Rename and adapt * Create FM_EXT_ReservePool_v1.t.sol * Rename to TokenVault * Adapt to not send native token * Address comments * Add: - Internal function to modifier - Trailing underscore to parameters - Gherkin to test - Cleaned up tests * Update FM_EXT_TokenVault_v1.t.sol --------- Co-authored-by: Zitzak <marvin88gr@gmail.com> Co-authored-by: Marvin <43185740+Zitzak@users.noreply.github.com> * Chore: Clean up TokenVault * Feat: Add FM_BC_BondingSurface_Redeemable_Repayer_Seizable_v1 (#666) * Create Reservepool Contract * Rename and adapt * Create FM_EXT_ReservePool_v1.t.sol * Topos FM * remove liquidity vault controller contracts * Rename to TokenVault * Adapt to not send native token * Rename to Token Vault * Address comments * Outcomment Burn tests for now * Create setTokenVault * Fix most Burn functions * AddSpentAllowance to ERC20Issuance * Add _spendAllowance to BondingCurveBase * Add _spendAllowance to BurnIssuanceFor * Fix Dependency Issue * Rename Redeemable to Redeeming * Create FM_BC_BondingSurface_Redeeming_v1.sol * WIP * Add: - Internal function to modifier - Trailing underscore to parameters - Gherkin to test - Cleaned up tests * restructure contracts * Add tag for testing file split * Setup seperate Tests * Update Base BondingSurface test * Rename Mock * Remove Tests that are included in the base contract * Remove parameters from BondingSurface Struct * Go over comments * Address comments * Address Comments * Adapt Naming of events * Adding Restricted to the name * Update comments * resolve comments in implementations * Fix: Finalize moving of error into funding manager base * Fix: Typos in ERC20Issuance * Chore: Start cleanup of FM_BC's * Fix: Resolve failing test Was mismatched with implementation, didn't cover the address(this) case. --------- Co-authored-by: FHieser <felix_hieser@web.de> Co-authored-by: Felix Hieser <47272854+FHieser@users.noreply.github.com> Co-authored-by: Marvin Kruse <marvinkruse@users.noreply.github.com> * Sc 720 expand unit tests for topos related modules (#676) * Add empty BondingSurface test file * Remove tokenVault from init The tokenvault cant be set during init because is created simultaneously to the BondingCurve contract. It has to be added via Dependency Injection * Remove unnecessary overrides * Add and adapt tests * Adapt inheritance of functions * Update Comments * Update Tests * Update fmt * Add event to projectFeeCollected * Address comments of PR --------- Co-authored-by: Zitzak <marvin88gr@gmail.com> * Sc 828 address omega review comments (#680) * Add empty BondingSurface test file * Remove tokenVault from init The tokenvault cant be set during init because is created simultaneously to the BondingCurve contract. It has to be added via Dependency Injection * Remove unnecessary overrides * Add and adapt tests * Adapt inheritance of functions * Update Comments * Update Tests * Update fmt * Add event to projectFeeCollected * Address comments of PR * Revert if redeemamount reaches Min Reserve * Check for Min Reserve in TransferOrchestrator Token * Add MinReserveCheck to transferRepayment * Rename Modifier * Remove time casting * Enable BuyFee * Use function that can be overriden * Add gap * Make Min_Reserves variable based on Token Decimals * Use SpotPrice to get StaticPrice * adapt based on comments * Update FM_BC_BondingSurface_Redeeming_v1.t.sol --------- Co-authored-by: Zitzak <marvin88gr@gmail.com> * Refactor Update the BondingSurface implementation to represent the code of conduct * Add empty BondingSurface test file * Remove tokenVault from init The tokenvault cant be set during init because is created simultaneously to the BondingCurve contract. It has to be added via Dependency Injection * Remove unnecessary overrides * Add and adapt tests * Adapt inheritance of functions * Update Comments * Update Tests * Update fmt * Add event to projectFeeCollected * Address comments of PR * Revert if redeemamount reaches Min Reserve * Check for Min Reserve in TransferOrchestrator Token * Add MinReserveCheck to transferRepayment * Rename Modifier * Remove time casting * Enable BuyFee * Use function that can be overriden * Add gap * Make Min_Reserves variable based on Token Decimals * Use SpotPrice to get StaticPrice * adapt based on comments * Update FM_BC_BondingSurface_Redeeming_v1.t.sol * WIP push * Rename seizable * Make state internal * Rename Parameters and Return Values * Rename Contracts * Adapt to code of conduct * Adapting Interfaces to have Contract Overview Documentation * Adapt Sectioning * Adapt Mocks and Tests to parameter conventions * Remove Todos/notes * Adapt merge aftermath * Update BondingSurface.sol * Rename to prevent shadow * Adapt remapping * Address comments * Adapt init * Update inverter standard versioning * Delete FM_BC_BondingSurface_RedeemingV1_exposed.sol * Adapt Contract Naming Standards * Fix naming references * Fix test * Update naming reference * Adapt modifier to call internal function * Adapt naming of internal function * update modifier Naming * Adapt tests * Adapt naming internal functions * Adapt Parameters * Adapt naming of function state * Adapt sections * Update Interface Inheritance * Update versioning * Update natspec to be rule compliant * Fix naming * Delete ILiquidityVaultProviderWhitelist_v1.sol * Remove Liquidity Vault Controller Interface * Rename Modifier * Add solmate as lib * Revert "Add solmate as lib" This reverts commit 050b9d2. * Remove comment * Adapt natspec * Apply comment suggestions from code review Co-authored-by: Marvin Kruse <marvinkruse@users.noreply.github.com> * Adapt internal function state * Add . to comments * Adapt spacing in natspec comments * Adapt spacing * Replace @dev with @notice * Adapt comment length * Adapt naming * Address Comments * Adapt Version * Reposition MIN_RESERVE * Adapt sectioning * Update more sectioning * Add title section and increment version --------- Co-authored-by: Zitzak <marvin88gr@gmail.com> Co-authored-by: Marvin Kruse <marvinkruse@users.noreply.github.com> * Add before and afterSell functionalities * Add new contracts to scripts * Rename Files to contain .t * Add BondingSurface E2E * Use ModuleRole * Finish e2e tests * Add Documentation * Adapt the Deployment Parameters * Create Contract Template.md * 4/5 tests passing * Create BondingSurface.md * Fix missing tests * update comment about eror tolerance * refactor: resolve renaming mismatch in contracts * fix: resolve RPC issue on tests --------- Co-authored-by: Felix Hieser <47272854+FHieser@users.noreply.github.com> Co-authored-by: Zitzak <marvin88gr@gmail.com> Co-authored-by: Marvin <43185740+Zitzak@users.noreply.github.com> Co-authored-by: FHieser <felix_hieser@web.de> Co-authored-by: 0xNuggan <82726722+0xNuggan@users.noreply.github.com>

@dev

* [SC-213] Feat: Reserve Vault (#665) * Create Reservepool Contract * Rename and adapt * Create FM_EXT_ReservePool_v1.t.sol * Rename to TokenVault * Adapt to not send native token * Address comments * Add: - Internal function to modifier - Trailing underscore to parameters - Gherkin to test - Cleaned up tests * Update FM_EXT_TokenVault_v1.t.sol --------- Co-authored-by: Zitzak <marvin88gr@gmail.com> Co-authored-by: Marvin <43185740+Zitzak@users.noreply.github.com> * Chore: Clean up TokenVault * Feat: Add FM_BC_BondingSurface_Redeemable_Repayer_Seizable_v1 (#666) * Create Reservepool Contract * Rename and adapt * Create FM_EXT_ReservePool_v1.t.sol * Topos FM * remove liquidity vault controller contracts * Rename to TokenVault * Adapt to not send native token * Rename to Token Vault * Address comments * Outcomment Burn tests for now * Create setTokenVault * Fix most Burn functions * AddSpentAllowance to ERC20Issuance * Add _spendAllowance to BondingCurveBase * Add _spendAllowance to BurnIssuanceFor * Fix Dependency Issue * Rename Redeemable to Redeeming * Create FM_BC_BondingSurface_Redeeming_v1.sol * WIP * Add: - Internal function to modifier - Trailing underscore to parameters - Gherkin to test - Cleaned up tests * restructure contracts * Add tag for testing file split * Setup seperate Tests * Update Base BondingSurface test * Rename Mock * Remove Tests that are included in the base contract * Remove parameters from BondingSurface Struct * Go over comments * Address comments * Address Comments * Adapt Naming of events * Adding Restricted to the name * Update comments * resolve comments in implementations * Fix: Finalize moving of error into funding manager base * Fix: Typos in ERC20Issuance * Chore: Start cleanup of FM_BC's * Fix: Resolve failing test Was mismatched with implementation, didn't cover the address(this) case. --------- Co-authored-by: FHieser <felix_hieser@web.de> Co-authored-by: Felix Hieser <47272854+FHieser@users.noreply.github.com> Co-authored-by: Marvin Kruse <marvinkruse@users.noreply.github.com> * Sc 720 expand unit tests for topos related modules (#676) * Add empty BondingSurface test file * Remove tokenVault from init The tokenvault cant be set during init because is created simultaneously to the BondingCurve contract. It has to be added via Dependency Injection * Remove unnecessary overrides * Add and adapt tests * Adapt inheritance of functions * Update Comments * Update Tests * Update fmt * Add event to projectFeeCollected * Address comments of PR --------- Co-authored-by: Zitzak <marvin88gr@gmail.com> * Sc 828 address omega review comments (#680) * Add empty BondingSurface test file * Remove tokenVault from init The tokenvault cant be set during init because is created simultaneously to the BondingCurve contract. It has to be added via Dependency Injection * Remove unnecessary overrides * Add and adapt tests * Adapt inheritance of functions * Update Comments * Update Tests * Update fmt * Add event to projectFeeCollected * Address comments of PR * Revert if redeemamount reaches Min Reserve * Check for Min Reserve in TransferOrchestrator Token * Add MinReserveCheck to transferRepayment * Rename Modifier * Remove time casting * Enable BuyFee * Use function that can be overriden * Add gap * Make Min_Reserves variable based on Token Decimals * Use SpotPrice to get StaticPrice * adapt based on comments * Update FM_BC_BondingSurface_Redeeming_v1.t.sol --------- Co-authored-by: Zitzak <marvin88gr@gmail.com> * Refactor Update the BondingSurface implementation to represent the code of conduct * Add empty BondingSurface test file * Remove tokenVault from init The tokenvault cant be set during init because is created simultaneously to the BondingCurve contract. It has to be added via Dependency Injection * Remove unnecessary overrides * Add and adapt tests * Adapt inheritance of functions * Update Comments * Update Tests * Update fmt * Add event to projectFeeCollected * Address comments of PR * Revert if redeemamount reaches Min Reserve * Check for Min Reserve in TransferOrchestrator Token * Add MinReserveCheck to transferRepayment * Rename Modifier * Remove time casting * Enable BuyFee * Use function that can be overriden * Add gap * Make Min_Reserves variable based on Token Decimals * Use SpotPrice to get StaticPrice * adapt based on comments * Update FM_BC_BondingSurface_Redeeming_v1.t.sol * WIP push * Rename seizable * Make state internal * Rename Parameters and Return Values * Rename Contracts * Adapt to code of conduct * Adapting Interfaces to have Contract Overview Documentation * Adapt Sectioning * Adapt Mocks and Tests to parameter conventions * Remove Todos/notes * Adapt merge aftermath * Update BondingSurface.sol * Rename to prevent shadow * Adapt remapping * Address comments * Adapt init * Update inverter standard versioning * Delete FM_BC_BondingSurface_RedeemingV1_exposed.sol * Adapt Contract Naming Standards * Fix naming references * Fix test * Update naming reference * Adapt modifier to call internal function * Adapt naming of internal function * update modifier Naming * Adapt tests * Adapt naming internal functions * Adapt Parameters * Adapt naming of function state * Adapt sections * Update Interface Inheritance * Update versioning * Update natspec to be rule compliant * Fix naming * Delete ILiquidityVaultProviderWhitelist_v1.sol * Remove Liquidity Vault Controller Interface * Rename Modifier * Add solmate as lib * Revert "Add solmate as lib" This reverts commit 050b9d2. * Remove comment * Adapt natspec * Apply comment suggestions from code review Co-authored-by: Marvin Kruse <marvinkruse@users.noreply.github.com> * Adapt internal function state * Add . to comments * Adapt spacing in natspec comments * Adapt spacing * Replace @dev with @notice * Adapt comment length * Adapt naming * Address Comments * Adapt Version * Reposition MIN_RESERVE * Adapt sectioning * Update more sectioning * Add title section and increment version --------- Co-authored-by: Zitzak <marvin88gr@gmail.com> Co-authored-by: Marvin Kruse <marvinkruse@users.noreply.github.com> * Add before and afterSell functionalities * Add new contracts to scripts * Rename Files to contain .t * Add BondingSurface E2E * Use ModuleRole * Finish e2e tests * Add Documentation * Adapt the Deployment Parameters * Create Contract Template.md * 4/5 tests passing * Create BondingSurface.md * Fix missing tests * update comment about eror tolerance * refactor: resolve renaming mismatch in contracts * fix: resolve RPC issue on tests --------- Co-authored-by: Felix Hieser <47272854+FHieser@users.noreply.github.com> Co-authored-by: Zitzak <marvin88gr@gmail.com> Co-authored-by: Marvin <43185740+Zitzak@users.noreply.github.com> Co-authored-by: FHieser <felix_hieser@web.de> Co-authored-by: 0xNuggan <82726722+0xNuggan@users.noreply.github.com>

Zitzak and others added 19 commits October 15, 2024 11:21

Add empty BondingSurface test file

f75b140

Remove tokenVault from init

6e47098

The tokenvault cant be set during init because is created simultaneously to the BondingCurve contract. It has to be added via Dependency Injection

Remove unnecessary overrides

bedd589

Add and adapt tests

2f48f53

Adapt inheritance of functions

2e8f498

Update Comments

d41c69a

Update Tests

ed14de9

Update fmt

995aa24

Add event to projectFeeCollected

3ba8a23

Address comments of PR

fbbf824

Revert if redeemamount reaches Min Reserve

244538c

Check for Min Reserve in TransferOrchestrator Token

8e700c0

Add MinReserveCheck to transferRepayment

f995fb7

Rename Modifier

200a0af

Remove time casting

bc05768

Enable BuyFee

cad4d30

Use function that can be overriden

202f145

Add gap

da730ac

Make Min_Reserves variable based on Token Decimals

a1655e2

FHieser requested a review from Zitzak October 24, 2024 11:26

Use SpotPrice to get StaticPrice

6aba226

Zitzak changed the base branch from main to feature/ToposFundingManager October 25, 2024 09:40

Zitzak requested changes Oct 25, 2024

View reviewed changes

FHieser added 3 commits October 25, 2024 14:25

adapt based on comments

ac07efb

Update FM_BC_BondingSurface_Redeeming_v1.t.sol

b920788

Merge branch 'feature/ToposFundingManager' into sc-828-address-omega-…

3ca1803

…review-comments

Zitzak approved these changes Nov 1, 2024

View reviewed changes

FHieser merged commit bf1b120 into feature/ToposFundingManager Nov 1, 2024

FHieser deleted the sc-828-address-omega-review-comments branch November 1, 2024 08:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sc 828 address omega review comments#680

Sc 828 address omega review comments#680
FHieser merged 23 commits intofeature/ToposFundingManagerfrom
sc-828-address-omega-review-comments

FHieser commented Oct 24, 2024 •

edited

Loading

Uh oh!

linear bot commented Oct 24, 2024

Uh oh!

Zitzak Oct 25, 2024

Uh oh!

FHieser Oct 25, 2024

Uh oh!

Zitzak Oct 25, 2024

Uh oh!

Zitzak Oct 25, 2024

Uh oh!

FHieser Oct 25, 2024

Uh oh!

Zitzak Oct 25, 2024

Uh oh!

Zitzak Oct 25, 2024

Uh oh!

FHieser Oct 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	// MIN_RESERVE is in dependency to the decimals of the workflow token
	// MIN_RESERVE is relational to the decimals of the workflow/colalteral token


		token().safeTransfer(to, amount);

		if (MIN_RESERVE > token().balanceOf(address(this))) {

Conversation

FHieser commented Oct 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linear bot commented Oct 24, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

FHieser commented Oct 24, 2024 •

edited

Loading