Conversation
bergusman
requested review from
andy-t-wang,
aurel-fr,
lukejmann,
murph,
paolodamico,
sideround and
tomislavhoman
as code owners
|
lets make some variable namings clearer - use |
There was a problem hiding this comment.
UsdVault::migrateseems to have a permit2 allowance call on the wrong token, or isn't converting units correctlyWldVault::migratedoesn't seem to account for the user earning interest on their legacy vault position, and will underfund the deposit into Morpho, resulting in WLD left over in their walletUsdVault::migratehas zero slippage tolerance on theredeemSDAICall. A small exchange rate difference in the time this takes to go on chain will result in it revertingfetch_sdai_balanceintransaction_usd_vault_migrateseems to be fetching the sdai balance on the wrong thing (user wallet, not their sDAI balance in the vault)
Less concerning:
- We don't need
networkpassing, everything is deployed on worldchain? - Need more docs and better naming throughout. Use
legacyto refer to the old vaults so we don't get mixed up with Morpho - Hardcode more addresses - there's only one WLD vault on Morpho for example - so client doesn't need to find all the addresses. Where would it get them from?
thomas-waite
left a comment
thomas-waite
left a comment
There was a problem hiding this comment.
Tests need to be more thorough with negative test cases added
| let morpho_shares_after = morpho_vault.balanceOf(safe_address).call().await?; | ||
| println!("MorphoVault shares after migration: {morpho_shares_after}"); | ||
|
|
||
| assert!( |
There was a problem hiding this comment.
These tests are a start, but they need to be way, way more thorough. They only test the main happy path.
We need negative test cases: test what happens if a user with zero balance in the vaults attempts a migration, test an asset/vault mismatch (attempting to migrate WLD to the USDC Morpho vault), test trying to migrate more than the user has in the legacy vault etc.
| /// ERC-20 approve for Permit2 contract | ||
| Permit2Approve = 138, | ||
| /// WLD Vault migration to ERC-4626 vault | ||
| WLDVaultMigration = 139, |
There was a problem hiding this comment.
| WLDVaultMigration = 139, | |
| WldVaultMigration = 139, |
nit. rust convention
| Ok(()) | ||
| } | ||
|
|
||
| pub async fn set_erc20_balance_with_slot<P>( |
There was a problem hiding this comment.
this seems quite useful, there were other places where we where doing this operation, perhaps worth updating it to use the same helper tool?
There was a problem hiding this comment.
We already have set_erc20_balance_for_safe but it is tied to slot 0 for example for USDC is not 0 slot
|
|
||
| /// Permit2 data for secure token transfers. | ||
| #[derive(Debug, Clone)] | ||
| pub struct Permit2Data { |
There was a problem hiding this comment.
don't we already have a permit2 module with the relevant definitions?
There was a problem hiding this comment.
We not have permit2 structure with signature, I composed it together because we have some clippy function arguments limitation
|
|
||
| /// Represents a USD Vault migration transaction bundle. | ||
| #[derive(Debug)] | ||
| pub struct UsdLegacyVault { |
There was a problem hiding this comment.
This struct seems the same as a transaction, do we need another definition?
5 participants
This PR introduces two new transaction types for WLD and USD vaults migration to ERC4626 vaults.