Cleanup & small refactor

[xklob]

No description provided.

[japes12345]

THIS PR

1. Line 111 - making sure we’re using fill on the way in - then wherever else that impacts downstream as opposed to oracle.
2. Change length to duration
3. Rest of validateCollarOpts todo
4. removeLiquidityFromSlot - transferFrom ERC20 to make sure marketmaker is paid out
5. Need to change positionNet check to be vault bc engine is not a passthrough
6. Add in duration to pool lookup

COMMENTS

• Are we requiring that liquidity must be added above 100% / 10000?
• Let’s mature vs uniswap 10 minute twap “oracle” instead of chainlink
• Make sure UUIDs are precomputable based on keccak and none for vaultManager
• createTo ( vaultManager)
• Make Engine auth-able (in case) just bc is currently external
• Edge case for previewRedeem to ensure maturity price actually used
• J.Paul to finish

LATER

• Real Uniswap hookup
• Test tick size
• Eventually upgradeable
• CEI
• Ensuring johnny come latelys aren’t rewarded
• Custom errors + more requires (coming)
• Add fees (50bps - 100bps) - changeable via governance - replace tokenomics
• Caleb to Finish Keepers for maturities (April/May)
• Finalize security (May)
• Start Audit (May)
• Caleb to write new docs before demo day (May)
• Probably no intents - not get distracted

[japes12345]

final comment on broad solidity!

NEW QS FOR CALEB

• How are we planning to implement getHistoricalAssetPrice and current - ideally uni oracle
• Line 210 - Confused why 1e32 - not 18 sum not 1e4 of 10000 tick - how do you get there
• In all this math shouldn’t we be consulting erc20.decimals to know for sure how to handle it vs assuming 1e18 (also we should add tests that handle weird decimals like 8 and 10
• Line 237 when does the money actually move / transfer? Does the pool take it?
• I dont get how the finalize position call must be made from the engine, seems like the vault is calling it via either the user or marketmaker, wouldn’t that engine part just revert
• Seems like we rely on previewRedeem for finalize/redeem so want to make sure that is right

[xklob]

Todo Items (This PR)

• Line 111 - making sure we’re using fill on the way in - then wherever else that impacts downstream as opposed to oracle.
• Change length to duration
• Rest of validateCollarOpts todo
• removeLiquidityFromSlot - transferFrom ERC20 to make sure marketmaker is paid out
• Need to change positionNet check to be vault bc engine is not a passthrough
• Add in duration to pool lookup
• Custom errors + more requires (coming)

Comments

• Are we requiring that liquidity must be added above 100% / 10000?
• Let’s mature vs uniswap 10 minute twap “oracle” instead of chainlink
• Make sure UUIDs are precomputable based on keccak and none for vaultManager
• create2 ( vaultManager)

All of the above will be added in a later PR.

• Make Engine auth-able (in case) just bc is currently external

Engine methods currently have auth. Need to add more tests for this though.

• Edge case for previewRedeem to ensure maturity price actually used

Will address in later PR.

Todo Items (Later)

• Real Uniswap Hookup
• Test tick size
• Eventually upgradeable
• CEI
• Ensuring johnny come latelys aren’t rewarded
• Add fees (50bps - 100bps) - changeable via governance - replace tokenomics
• Caleb to Finish Keepers for maturities (April/May)
• Finalize security (May)
• Caleb to write new docs before demo day (May)
• Probably no intents - not get distracted

Additional Comments

• How are we planning to implement getHistoricalAssetPrice and current - ideally uni oracle

Uni TWAP is current plan.

• Line 210 - Confused why 1e32 - not 18 sum not 1e4 of 10000 tick - how do you get there

1e32 was just an arbitrarily large number I chose to avoid small-math errors. Will remove this in a later pass and do a more formal math-specific pass, eg solmate.

• In all this math shouldn’t we be consulting erc20.decimals to know for sure how to handle it vs assuming 1e18 (also we should add tests that handle weird decimals like 8 and 10

No need for erc20.decimals, it's actually better to do this in raw wei units. That's also why we don't need to handle weird cases like erc20s with 8 or 10 decimals. Decimals is just a frontend display unit - it doesn't actually impact the underlying math at all.

• Line 237 when does the money actually move / transfer? Does the pool take it?

Vault Manager calls pool.finalizePosition, and the pool will either transfer money to the vault manager if needed, or pull money from the vault manager if needed.

• I dont get how the finalize position call must be made from the engine, seems like the vault is calling it via either the user or marketmaker, wouldn’t that engine part just revert

Finalize position call isn't made to or from the engine; the contracts only check with the engine to make sure someone calling them is who they claim to be.

• Seems like we rely on previewRedeem for finalize/redeem so want to make sure that is right

100%

[xklob]

LGTM