Skip to content

Unlist Market from eMode Pools #650

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
web3rover wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Unlist Market from eMode Pools #650

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ec98520
fix: remove market from emode group when unlisting
web3rover Nov 13, 2025
84c3d39
fix: optimisations
web3rover Nov 16, 2025
2e512eb
fix: fixed tests
web3rover Nov 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 37 additions & 22 deletions contracts/Comptroller/Diamond/facets/MarketFacet.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,18 @@ contract MarketFacet is IMarketFacet, FacetBase {
require(_market.collateralFactorMantissa == 0, "collateral factor is not 0");

_market.isListed = false;
Copy link
Copy Markdown
Contributor

@GitGuru7 GitGuru7 Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@web3rover web3rover Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed


for (uint96 i = lastPoolId; i > corePoolId; i--) {
address[] memory markets = getPoolVTokens(uint96(i));

for (uint256 j = 0; j < markets.length; j++) {
if (markets[j] == market) {
Copy link
Copy Markdown
Contributor

@GitGuru7 GitGuru7 Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@fred-venus fred-venus Nov 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree, let's avoid looping here which could waste ton of gas

PoolMarketId.wrap(bytes32((uint256(poolId) << 160) | uint160(vToken)));

Copy link
Copy Markdown
Contributor Author

@web3rover web3rover Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

_removePoolMarket(i, market);
break;
}
}
}

emit MarketUnlisted(market);

return uint256(Error.NO_ERROR);
Expand Down Expand Up @@ -409,27 +421,7 @@ contract MarketFacet is IMarketFacet, FacetBase {
*/
function removePoolMarket(uint96 poolId, address vToken) external {
ensureAllowed("removePoolMarket(uint96,address)");

if (poolId == corePoolId) revert InvalidOperationForCorePool();
PoolMarketId index = getPoolMarketIndex(poolId, vToken);
if (!_poolMarkets[index].isListed) {
revert PoolMarketNotFound(poolId, vToken);
}

address[] storage assets = pools[poolId].vTokens;

uint256 length = assets.length;
for (uint256 i; i < length; i++) {
if (assets[i] == vToken) {
assets[i] = assets[length - 1];
assets.pop();
break;
}
}

delete _poolMarkets[index];

emit PoolMarketRemoved(poolId, vToken);
_removePoolMarket(poolId, vToken);
}

/**
Expand Down Expand Up @@ -507,7 +499,7 @@ contract MarketFacet is IMarketFacet, FacetBase {
* @custom:error PoolDoesNotExist Reverts if the given pool ID do not exist.
* @custom:error InvalidOperationForCorePool Reverts if called on the Core Pool.
*/
function getPoolVTokens(uint96 poolId) external view returns (address[] memory) {
function getPoolVTokens(uint96 poolId) public view returns (address[] memory) {
if (poolId > lastPoolId) revert PoolDoesNotExist(poolId);
if (poolId == corePoolId) revert InvalidOperationForCorePool();
return pools[poolId].vTokens;
Expand Down Expand Up @@ -697,6 +689,29 @@ contract MarketFacet is IMarketFacet, FacetBase {
emit PoolMarketInitialized(poolId, vToken);
}

function _removePoolMarket(uint96 poolId, address vToken) internal {
if (poolId == corePoolId) revert InvalidOperationForCorePool();
PoolMarketId index = getPoolMarketIndex(poolId, vToken);
if (!_poolMarkets[index].isListed) {
revert PoolMarketNotFound(poolId, vToken);
}

address[] storage assets = pools[poolId].vTokens;

uint256 length = assets.length;
for (uint256 i; i < length; i++) {
if (assets[i] == vToken) {
assets[i] = assets[length - 1];
assets.pop();
break;
}
}

delete _poolMarkets[index];

emit PoolMarketRemoved(poolId, vToken);
}

/**
* @notice Returns only the core risk parameters (CF, LI, LT) for a vToken in a specific pool.
* @dev If the pool is inactive, or if the vToken is not configured in the given pool and
Expand Down
5 changes: 5 additions & 0 deletions tests/hardhat/Comptroller/Diamond/assetListTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ describe("Comptroller: assetListTest", () => {

const receipt = await comptroller.connect(customer).unlistMarket(unlistToken.address);
expect(receipt).to.emit(unitroller, "MarketUnlisted");
expect(receipt).to.emit(unitroller, "PoolMarketRemoved");
Copy link
Copy Markdown
Contributor

@GitGuru7 GitGuru7 Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Copy Markdown
Contributor Author

@web3rover web3rover Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed


const expectedError_ = expectedError || Error.NO_ERROR;
expect(reply).to.equal(expectedError_);
Expand Down Expand Up @@ -326,6 +327,10 @@ describe("Comptroller: assetListTest", () => {
true,
);

const newLabel = "test-pool";
await comptroller.createPool(newLabel);
await comptroller.addPoolMarkets([1], [OMG.address]);

await unlistAndCheckMarket(OMG, [BAT, ZRX], [OMG, BAT, ZRX]);
});

Expand Down
Loading