Mainnet WETH Bulker by scott-silver · Pull Request #611 · compound-finance/comet

scott-silver · 2022-11-01T18:50:36Z

Creating a new Bulker that is able to wrap/unwrap stETH.

Includes the following changes:

moves the Bulker into a bulkers/ subdirectory, renaming it to BaseBulker
creates a new MainnetBulker that inherits from BaseBulker and adds supplyStEthTo and withdrawStEthTo functions
converts the bulker action constants to strings/bytes32 to decrease the likelihood that bulkers will create actions with conflicting values
adds a handleAction function that contracts inheriting from BaseBulker can implement; by default it just reverts
scenarios for wrapping and unwrapping via the MainnetBulker

TODO:

fix unit tests
add scenario for BaseBulker that checks the UnhandledAction() error

kevincheng96 · 2022-11-01T20:34:43Z

contracts/bulkers/BaseBulker.sol

 }

-contract Bulker {
+contract BaseBulker {


Can't comment on line 35 so going to comment here instead. Should we go ahead and make the base even more generic by renaming weth to wrappedNativeToken?

Yeah, it's a good question. wrappedNativeToken will be the more accurate name once we're using the bulker for wrapped Matic and wrapped Avax.

The downside is that it will break the existing interface. With these changes, the existing, deployed Bulkers and the new MainnetBulker have the same interface (the MainnetBulker just adds a couple additional actions).

But if we update the constants to be ACTION_SUPPLY_NATIVE_TOKEN instead of ACTION_SUPPLY_ETH, then we'll need to either write deployment-specific scenarios or replace the deployed bulkers.

IMO it's fine to temporarily have both interfaces in the repo so scenarios can still use the old Bulker interface until we complete the migration on-chain. The cost of not making this change is that we'll either need a separate BaseBulker for the other chains or the bulker on other chains will incorrectly refer to WMATIC or WAVAX as WETH.

sounds good. updated all references to eth/weth to be nativeToken/wrappedNativeToken

kevincheng96 · 2022-11-01T20:36:12Z

contracts/bulkers/MainnetBulker.sol

+        address payable steth_,
+        address payable wsteth_
+    ) BaseBulker(admin_, weth_) {
+        steth = steth_;


You can derive this from wsteth.stETH().

good call; updated (and made the steth and wsteth address not payable)

Good catch, didn't catch the payable on my end

kevincheng96

Nice, the contract looks good so far! Just a few small comments

kevincheng96 · 2022-11-01T20:41:17Z

scenario/BulkerScenario.ts

+    );
+    const calldata = [supplyGalacticCreditsCalldata];
+    const actions = [
+      ethers.utils.formatBytes32String('ACTION_SUPPLY_GALACTIC_CREDITS')


👽: Comet doesn't support Galactic Credits?!

it's an outrage! just wait until the Emperor hears about this

contracts/bulkers/IWstETH.sol

jflatow

Awesome

kevincheng96

Looks good 💪

kevincheng96 · 2022-11-02T01:08:36Z

contracts/bulkers/MainnetBulker.sol

-        address payable wsteth_
-    ) BaseBulker(admin_, weth_) {
-        steth = steth_;
+        address payable wrappedNativeToken_,


nit: assuming this Bulker is for Ethereum mainnet, it should be fine to name this weth

yep, you're right. updated

* update Github Actions to include mainnet-eth * ETH-base bulker scenario * WstETHPriceFeed + tests (#600) * non-ETH and ETH bulker scenarios (all actions in one txn) * Mainnet WETH Bulker (#611)

* Update Github Actions to include mainnet-eth * ETH-base bulker scenario * WstETHPriceFeed + tests (#600) * Non-ETH and ETH bulker scenarios (all actions in one txn) * Mainnet WETH Bulker (#611) * Add a supply cap constraint and set initial caps to 0 for cWETHv3 * Update the collateral params based on Gauntlet recommendations (#628) https://hackmd.io/wncIvkFTReWUe2AMMK2ezA?view * Price feeds for WETH deployment + Bulker changes for OZ audit (#625) This PR implements and modifies price feeds to support the upcoming WETH deployment. The favored plan so far is to use ETH-denominated price feeds as opposed to USD price feeds, but stick with using 8 decimals for prices to avoid having to change the `Comet` and `Configurator` implementations. This would require: - A new wrapper price feed (`ScalingPriceFeed.sol`) that scales prices up or down to 8 decimals - A new `ConstantPriceFeed` that always returns 1e8 for the `WETH` base asset, since should always hold a 1:1 value with ETH - Modifications to the `WstETHPriceFeed` to return prices in terms of ETH instead of USD This is an alternative approach to #626, which is a more complex change but could be a better long-term solution. *Note: This PR also now contains the changes from #634 and #635, which address some suggestions made by OZ for their audit of `WstETHPriceFeed` and `Bulker`.*

Siroph79

test/helpers.ts

* Mainnet ETH Deployment * Update Github Actions to include mainnet-eth * ETH-base bulker scenario * WstETHPriceFeed + tests (#600) * Non-ETH and ETH bulker scenarios (all actions in one txn) * Mainnet WETH Bulker (#611) * Add a supply cap constraint and set initial caps to 0 for cWETHv3 * Update the collateral params based on Gauntlet recommendations (#628) https://hackmd.io/wncIvkFTReWUe2AMMK2ezA?view * Price feeds for WETH deployment + Bulker changes for OZ audit (#625) This PR implements and modifies price feeds to support the upcoming WETH deployment. The favored plan so far is to use ETH-denominated price feeds as opposed to USD price feeds, but stick with using 8 decimals for prices to avoid having to change the `Comet` and `Configurator` implementations. This would require: - A new wrapper price feed (`ScalingPriceFeed.sol`) that scales prices up or down to 8 decimals - A new `ConstantPriceFeed` that always returns 1e8 for the `WETH` base asset, since should always hold a 1:1 value with ETH - Modifications to the `WstETHPriceFeed` to return prices in terms of ETH instead of USD This is an alternative approach to #626, which is a more complex change but could be a better long-term solution. *Note: This PR also now contains the changes from #634 and #635, which address some suggestions made by OZ for their audit of `WstETHPriceFeed` and `Bulker`.* * Refactor deploy to support second markets (#639) * Refactor deploy to support second markets * Add a fromDep to deployment manager which can be used for sharing dependency contracts from another deployment * Always attempt to initialize storage if needed, and not necessarily as gov * Always deploy an initial implementation contract instead of using the factory marker * Only take gov admin actions if we own the cometAdmin * Expose the primitives necessary for a proposal to configurate later, if/when admin cannot * Add an env flag for migration constraint to skip non-migration scenarios * Defer reading current config in modern constraint (bugfix) * Add recipient to token sourcing blacklist * WETH deployment audit changes (#641) These are changes in response to the WETH deployment [audit](https://gist.github.com/andresbach/a01686b1ef8bdf51d46046f2cfc93307) conducted by OZ: - **L01 - Lack of input validation** - We originally avoided zero address checks because there are a vast amount of accidental addresses that can be set here and checking for a specific one seems unnecessary. However, we have reconsidered that position after seeing the reasons for including a zero address check listed in this [post](https://forum.openzeppelin.com/t/removing-address-0x0-checks-from-openzeppelin-contracts/2222/13). - **L02 - Improper implementation of Chainlink AggregatorV3Interface** - We updated the hard-coded return values for `roundId` and `answeredInRound` to be 1 to comply with the Chainlink specification. - **L04 - Incomplete/confusing documentation** - We updated the documentation based on the provided suggestions. Our full audit responses are [here](https://docs.google.com/document/d/1A8TrUdTtD7DXr1U5HOqSS1Xl81rjBZdlDC7LS4No4Bw). * Spider USDC deployment before calling `fromDep` (#644) We need to spider the `USDC` deployment before calling `fromDep` on it so all the aliases are populated in the cache. Otherwise, `fromDep` may try to import an `undefined` contract. There are many approaches to this and this PR takes the simplest approach of spidering in the deploy script. Another approach could be to handle the spidering directly in `fromDep`. * Convert from scientific notation to bigint in configuration (#643) This PR adds a new `ScientificNotation` string type to `NetworkConfiguration` that is required for certain fields (e.g. `supplyCap`, `baseMinForRewards`). The configuration parser will safely convert these values into bigints without losing precision; with one caveat, scientific notations that use decimals in the coefficient still run the risk of precision loss because those are converted directly to javascript numbers. * Explicit supply cap constraint for failing bulker scen (#648) * Add tracking index bounds unit tests for WETH (#645) * Update params and rate model final numbers for deployment * OnChainLiquidator (#642) Update the reference liquidator to source from more pools and generally be more robust * Fix fromDep to spider the dependency and load the contract from it (#652) * bump targetReserves in LiquidationBotScenarios (#653) * delete CURVE_REGISTRY (#654) * OnChainLiquidator, not Liquidator (#655) * Combined model based on Gauntlet's first and second recommendations (#656) * Wrap and unwrap native token as base asset in Bulker (#659) * Add tests for supply native token; clean up * Change supplyStETH to use stETH amount instead of wstETH amount; add scenarios * Tweak comments * Supplying uint256.max collateral reverts in WETH deposit not Comet * Modified deployment roots from GitHub Actions * Proposal to enable cWETHv3 market (#608) * Proposal to enable cWETHv3 market * Modify proposal to add rewards * Update the supply caps based on Gauntlet recommendations (#629) https://hackmd.io/wncIvkFTReWUe2AMMK2ezA?view * Refactor proposal to make market live (#640) * If there are migrations, don't run the non-migrations This allows a proposal branch to pass CI if the migration is required and is generally the intention for migration branches. However an env var is still supported to override and force running the non-migration, too. * Final changes to supply caps; proposal text; WETH transfer amt (#662) Also copy bulker to usdc roots and fix scen test fixes * Deploy cWETHv3 to Goerli (#646) This PR adds a new `goerli-weth` base and deploy script for the Goerli cWETHv3 deploy. The deploy script is very similar to the mainnet cWETHv3 script. The main differences are: - Deploying price feeds for stETH / ETH and cbETH / ETH - Pulling in extra testnet/cross-chain contracts such as `fauceteer` and `fxRoot` using `fromDep` - Cloning over `cbETH` from mainnet - Minting `cbETH` to `fauceteer` * Modified migration from GitHub Actions Co-authored-by: Kevin Cheng <kevincheng96@hotmail.com> Co-authored-by: scott-silver <scott@compound.finance> Co-authored-by: GitHub Actions Bot <>

scott-silver added 10 commits October 31, 2022 15:14

move Bulker to BaseBulker; create MainnetBulker

0992998

update mainnet deploy script

d337d6a

add relations

c8596dd

update deploy script

54006be

add MainnetBulkerScenario

24ea3c5

implement withdrawStEthTo

19f0034

create separate IWstETH contract

0b0804f

withdraw scenario

c88b730

clean up scenarios

057e320

check stETH balance, not wstETH balance

231120f

scott-silver requested a review from kevincheng96 November 1, 2022 18:50

scott-silver added 3 commits November 1, 2022 11:56

lint

06f15d4

update makeBulker

4023282

UnhandledAction scenario

3be0c7e

kevincheng96 reviewed Nov 1, 2022

View reviewed changes

scott-silver added 2 commits November 1, 2022 14:00

move UnhandledAction check

2928e70

derive stETH address

6580659

scott-silver marked this pull request as ready for review November 1, 2022 21:14

scott-silver changed the title ~~[draft] Mainnet WETH Bulker~~ Mainnet WETH Bulker Nov 1, 2022

scott-silver requested review from hayesgm and jflatow November 1, 2022 21:15

scott-silver mentioned this pull request Nov 1, 2022

Mainnet WETH Deployment #587

Merged

6 tasks

jflatow reviewed Nov 1, 2022

View reviewed changes

contracts/bulkers/IWstETH.sol Show resolved Hide resolved

jflatow approved these changes Nov 1, 2022

View reviewed changes

scott-silver added 3 commits November 1, 2022 14:54

update Mumbai deploy script

959aa7b

move IWstETH.sol up one level

9893df5

rename weth to wrappedNativeToken

43e3093

exempt mumbai deployment

fd89259

kevincheng96 approved these changes Nov 2, 2022

View reviewed changes

weth not wrappedNativeToken for MainnetBulker

5424050

scott-silver merged commit b4ccfec into jflatow/mainnet-eth Nov 2, 2022

Siroph79 reviewed Jan 9, 2023

View reviewed changes

Conversation

scott-silver commented Nov 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scott-silver Nov 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

kevincheng96 left a comment

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!

Uh oh!

jflatow left a comment

Choose a reason for hiding this comment

Uh oh!

kevincheng96 left a comment

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!

This comment was marked as spam.

Uh oh!

Siroph79 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

scott-silver commented Nov 1, 2022 •

edited

Loading

scott-silver Nov 1, 2022 •

edited

Loading