Conversation
GitGuru7
left a comment
GitGuru7
left a comment
There was a problem hiding this comment.
Overall LGTM! Left a couple of nitpicks please let me know if they make sense to you.
| for (uint256 j = 0; j < markets.length; j++) { | ||
| if (markets[j] == market) { |
There was a problem hiding this comment.
I understand this is to avoid a revert in _removePoolMarket if the market is not already listed in eMode. I would prefer using a simple check like if (_poolMarkets[index].isListed) similar to what we have in _removePoolMarket instead of looping over all poolMarkets.
There was a problem hiding this comment.
agree, let's avoid looping here which could waste ton of gas
PoolMarketId.wrap(bytes32((uint256(poolId) << 160) | uint160(vToken)));
| @@ -217,6 +217,18 @@ contract MarketFacet is IMarketFacet, FacetBase { | |||
| require(_market.collateralFactorMantissa == 0, "collateral factor is not 0"); | |||
|
|
|||
| _market.isListed = false; | |||
There was a problem hiding this comment.
Nit: We currently don’t allow adding a market to a pool that isn’t listed in the Core Pool. It might be better to set _market.isListed = false after removing it from all pools (delisting from eMode). Functionally it doesn’t change behavior, but it aligns the logic more clearly.
|
|
||
| const receipt = await comptroller.connect(customer).unlistMarket(unlistToken.address); | ||
| expect(receipt).to.emit(unitroller, "MarketUnlisted"); | ||
| expect(receipt).to.emit(unitroller, "PoolMarketRemoved"); |
There was a problem hiding this comment.
I see we already added an event check in the test. Could we also include a simple test to verify that an unlisted market is no longer present in any eMode pool ?
|
Also, pls ask them to audit this part as well |
|
fred-venus
force-pushed
the
develop
branch
6 times, most recently
from
f8786ac to
d1378e1
Compare
3 participants
Description
Resolves #
Checklist