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

Add erc4626 #1170

Draft
wants to merge 51 commits into
base: main
Choose a base branch
from
Draft

Conversation

andrew-fleming
Copy link
Collaborator

@andrew-fleming andrew-fleming commented Oct 1, 2024

WIP

Fixes #???

PR Checklist

  • Tests
  • Documentation
  • Added entry to CHANGELOG.md
  • Tried the feature on a public network

...still a WIP. The tests need to be refactored and improved. The idea was to make sure the logic works as expected so there's a standard (from the openzeppelin solidity tests) for any and all changes moving forward.

Some things to note and discuss:

Decimals

EIP4626 states at the end of the doc:

Although the convertTo functions should eliminate the need for any use of an EIP-4626 Vault’s decimals variable, it is still strongly recommended to mirror the underlying token’s decimals if at all possible, to eliminate possible sources of confusion and simplify integration across front-ends and for other off-chain users.

The OpenZeppelin solidity implementation checks for the underlying asset's tokens upon construction with a try/catch which defaults to 18 decimals if query fails. Given Starknet's current status with try/catch (✌️ dual dispatchers), this doesn't seem like a viable approach. If the vault is for an undeployed token, the deployment will fail. This PR proposes that the decimals (both underlying decimals and offset decimals) are explicitly defined by the contract through the ImmutableConfig.

Math

u512 Precision math for multiply and divide (u256_mul_div)

This PR leverages the corelib's wide_mul and u512_safe_div_rem_by_u256 for mathematical precision. This is set as a tmp solution and should be scrutinized further. More tests should be provided and we should look for ways to optimize.

Exponentiation

This PR requires exponentiation for converting to shares and assets. The current implementation is the brute force formula. We can definitely improve this.

FeeConfigTrait

This PR proposes to utilize FeeConfigTrait (which is really like a hook) for contracts to integrate entry and exit fees for preview_ fns. The state-changing methods of ERC4626 rely on preview_ to determine the number of assets or shares to exchange.

Another approach that can reduce the verbosity of the traits/hooks is to have a single adjust_assets_or_shares function that accepts an ExchangeType.

    #[derive(Drop, Copy)]
    pub enum ExchangeType {
        Deposit,
        Withdraw,
        Mint,
        Redeem
    }

    pub trait ERC4626HooksTrait<TContractState> {
        (...)

        fn adjust_assets_or_shares(
            self: @ComponentState<TContractState>, exchange_type: ExchangeType, raw_amount: u256
        ) -> u256 {
            raw_amount
        }
    }

    /// Component method example
    fn preview_mint(self: @ComponentState<TContractState>, shares: u256) -> u256 {
        let raw_amount = self._convert_to_assets(shares, Rounding::Ceil);
        Hooks::adjust_assets_or_shares(self, ExchangeType::Deposit, raw_amount)
    }

The downside though is I think it's easy to misuse i.e.

#[starknet::contract]
mod Contract {
    (...)

    impl ERC4626HooksImpl of ERC4626Component::ERC4626HooksTrait<ContractState> {
        fn adjust_assets_or_shares(
            self: @ERC4626Component::ComponentState<ContractState>, exchange_type: ExchangeType, raw_amount: u256
        ) -> u256 {
            match exchange_type {
                ExchangeType::Mint => {
                    // do something
                },
                ExchangeType::Deposit => {
                    // do something
                },
                ExchangeType::Withdraw => {
                    // do something
                },
                ExchangeType::Redeem => {
                    // do something
                }
            }
        }
    }
}

IMO having a dedicated function for each exchange type is more difficult to mess up...but it's at the cost of some verbosity.

LimitsConfigTrait

This mirrors the FeeConfigTrait except that these target the limits on the max_ methods and return an Option so Option::None can point to the default. Same arguments apply for not having a single trait/hook with an ExchangeType parameter.

before_withdraw and after_deposit hooks

The before_withdraw and after_deposit hooks take inspiration from solmate's solidity implementation of erc4626. These hooks are where contracts can transfer fees calculated from the FeeConfigTrait in the implementing contract. See the Fees mock to see how this works in the proposed PR.

Copy link

codecov bot commented Oct 1, 2024

Codecov Report

Attention: Patch coverage is 96.58120% with 4 lines in your changes missing coverage. Please review.

Project coverage is 92.31%. Comparing base (7ac655c) to head (2e368fd).

Files with missing lines Patch % Lines
...s/token/src/erc20/extensions/erc4626/erc4626.cairo 95.78% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1170      +/-   ##
==========================================
+ Coverage   91.89%   92.31%   +0.41%     
==========================================
  Files          47       48       +1     
  Lines        1197     1314     +117     
==========================================
+ Hits         1100     1213     +113     
- Misses         97      101       +4     
Files with missing lines Coverage Δ
packages/utils/src/math.cairo 100.00% <100.00%> (ø)
...s/token/src/erc20/extensions/erc4626/erc4626.cairo 95.78% <95.78%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ac655c...2e368fd. Read the comment docs.

@akiraonstarknet
Copy link

@andrew-fleming I know this is draft PR, but when going through code, found something. In the FeeConfigTrait, the adjust_mint function says the input is shares, but preview_mint passes assets as input. Similarly adjust_redeem too.

Just pointing it out so that it doesn't confuse anyone. If its known, please ignore this comment.

@andrew-fleming
Copy link
Collaborator Author

I know this is draft PR, but when going through code, found something. In the FeeConfigTrait, the adjust_mint function says the input is shares, but preview_mint passes assets as input. Similarly adjust_redeem too.

@akiraonstarknet sorry for the delay, I was on holiday the past couple weeks. And yes, you are correct. The initial param is shares in those preview_ fns, but they're converted to assets before they're passed to the adjust_ fns. Thanks for the feedback 🙏

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