Add erc4626

[andrew-fleming]

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.

This was added to the corelib (starkware-libs/cairo#6694). Will update when released

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.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.33%. Comparing base (0676415) to head (8729faa).
Report is 10 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1170      +/-   ##
==========================================
- Coverage   92.26%   89.33%   -2.94%     
==========================================
  Files          59       81      +22     
  Lines        1811     3470    +1659     
==========================================
+ Hits         1671     3100    +1429     
- Misses        140      370     +230     

Files with missing lines	Coverage Δ
...s/token/src/erc20/extensions/erc4626/erc4626.cairo	100.00% <100.00%> (ø)
...token/src/erc20/extensions/erc4626/interface.cairo	100.00% <100.00%> (ø)
packages/utils/src/math.cairo	100.00% <100.00%> (ø)

... and 78 files with indirect coverage changes

---

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 86debd4...8729faa. Read the comment docs.

[ericnordelo]

Hey @andrew-fleming , the overall implementation looks great! I left a couple of comments, but my main suggestion is to simplify the fee mechanism.

With the current approach I feel it would be hard for users to follow how to implement fees, and is easy to misuse since adjust_[operation] functions doesn't have a percent let it clear on which percentages are used, or on how to transfer the fees on the Hooks accordingly from the amounts received as parameters.

I think we can do something like this to simplify the logic and make it easier to follow and use:

• Have a configurable FEE_DENOMINATOR in ImmutableConfig for precision default to 10000 (100 is 1%)
• Have only two functions in the FeeConfigTrait, entry_fee_numerator and exit_fee_numerator. Entry used for both deposit and mint, and exit for redeem and withdraw.
• Remove the Hooks trait and add the transfer fee logic in _deposit and _withdraw, with an if depending on the fee being greater than 0.
• Compute the fee and apply it where it needs to be applied (preview functions).
• Maybe add a couple of internal functions like entry_fee_set and exit_fee_set checking that the numerators are greater than 0.

I know this will add a couple of conditions and operations to the flow when the fees are not required (set to 0), but I think it will be negligible, and it will improve the UX a lot making it easier for users to implement a fee mechanism.

[ericnordelo]

The implementation of LimitConfigTrait must always be available, even if it is the default. We should rephrase this. I don't make any specific suggestions since you will write a better sentence for sure.

[ericnordelo]

Same as above.

[immrsd]

Good job! Left a couple of comments

[immrsd]

Should we clear the calldata that already exists to allow for a repeated call?

[andrew-fleming]

We shouldn't have to. There's only one reentrant call since we're setting the reentrant Type to No in the hooks after the initial call

[immrsd]

Shall we include a newline between a function and doc for the next one?

[immrsd]

Good job! Left several suggestions

[andrew-fleming]

With the current approach I feel it would be hard for users to follow how to implement fees, and is easy to misuse since adjust_[operation] functions doesn't have a percent let it clear on which percentages are used, or on how to transfer the fees on the Hooks accordingly from the amounts received as parameters.

So, I have an implementation with most of the suggested changes in a separate branch (I can push the changes in this PR if preferred). The impl includes:

• Fee denominator set in the ImmutableConfig
• Exit and entry fee numerators set in the FeeConfigTrait
• Exit and entry fee recipients set in the FeeConfigTrait
• Removal of hooks and instead integrate the fee transfer logic inside the _deposit and _withdraw fns

As mentioned off-line, I think this is too opinionated of an implementation. The suggested design (at least, my interpretation of it) makes it difficult to customize outside of standard entry and exit fees. A simple example where hooks would be very helpful is with implementing auto compounding rewards as seen here: https://github.com/ERC4626-Alliance/ERC4626-Contracts/blob/main/src/xERC4626.sol#L65-L74

Let me know if you guys disagree with my take or have ideas @ericnordelo @immrsd

[ericnordelo]

The PR looks great @andrew-fleming, my only concern is that it should be as hard to misuse as possible, so I'm pushing for more clarity on the comments on some places, but besides that, it looks ready.

Let's continue with the current approach regarding hooks from the points you've shared.

[ericnordelo]

Suggested change

	pub trait FeeConfigTrait<TContractState> {
	pub trait FeeConfigTrait<TContractState, +HasComponent<TContractState>> {

This is not strictly required, but is better semantically because it doesn't make much sense to implement this trait in a contract not implementing this component, and these guards against it.

Applies for the other traits too.

[andrew-fleming]

Done for fees, limits, and hooks! I know it's especially good to do it here, but it's probably worth going through the repo and seeing where else we can apply the component enforcement idea

[ericnordelo]

which conditions?

[ericnordelo]

Same here, from the comment is not clear what conditions need to be met.

[ericnordelo]

It is not clear at first glance what is the intention of these adjust functions or how to use them in the right way. We could add some explanation for that in comments for this trait or functions on how they must be used combined with fees for example, and if they have potentially other use cases the implementors (we) considered.

[ericnordelo]

This should be as hard to misuse as possible.

[ericnordelo]

It is not clear why someone would want to change the deposit preview value, this comment can be confusing to users trying to understand the component. An explanation of how this can be useful and how to potentially use it (fees) would be very helpful.

Apply for the other preview functions mentioning the FeeConfigTrait trait.