Skip to content
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

Jump to bottom

Zero distribution #878

Merged
merged 6 commits into from
Aug 2, 2023
Merged

Zero distribution #878

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a35ce5c
implement revenueTrader.returnTokens
tbrent Jul 31, 2023
b7d1986
test revenueTrader.returnTokens
tbrent Jul 31, 2023
7ffc7b5
fix comment
tbrent Aug 1, 2023
0f98260
Merge branch '3.0.0-rc5' into zero-distribution
tbrent Aug 1, 2023
1d1005b
remove args from non-emitted event
tbrent Aug 2, 2023
0115467
add untestable comment
tbrent Aug 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions contracts/interfaces/IRevenueTrader.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ interface IRevenueTrader is IComponent, ITrading {
/// @custom:interaction
function distributeTokenToBuy() external;

/// Return registered ERC20s to the BackingManager if distribution for tokenToBuy is 0
/// @custom:interaction
function returnTokens(IERC20[] memory erc20s) external;

/// Process some number of tokens
/// If the tokenToBuy is included in erc20s, RevenueTrader will distribute it at end of the tx
/// @param erc20s The ERC20s to manage; can be tokenToBuy or anything registered
Expand Down
20 changes: 20 additions & 0 deletions contracts/p0/RevenueTrader.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,26 @@ contract RevenueTraderP0 is TradingP0, IRevenueTrader {
_distributeTokenToBuy();
}

/// Return registered ERC20s to the BackingManager if distribution for tokenToBuy is 0
/// @custom:interaction
function returnTokens(IERC20[] memory erc20s) external notTradingPausedOrFrozen {
RevenueTotals memory revTotals = main.distributor().totals();
if (tokenToBuy == main.rsr()) {
require(revTotals.rsrTotal == 0, "rsrTotal > 0");
} else if (address(tokenToBuy) == address(main.rToken())) {
require(revTotals.rTokenTotal == 0, "rTokenTotal > 0");
} else {
revert("invalid tokenToBuy");
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not going to be covered with our default tests. So we can either deploy a standalone mock case with a RevTrader initialized to another tokenToBuy (not generally what we do in most tests as it would not be our "interface") or we add the untestable comment

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Adding the comment

}

// Return ERC20s to the BackingManager
for (uint256 i = 0; i < erc20s.length; i++) {
require(main.assetRegistry().isRegistered(erc20s[i]), "unregistered erc20");
address backingManager = address(main.backingManager());
erc20s[i].safeTransfer(backingManager, erc20s[i].balanceOf(address(this)));
}
}

/// Process some number of tokens
/// @param erc20s The ERC20s to manage; can be tokenToBuy or anything registered
/// @param kinds The kinds of auctions to launch: DUTCH_AUCTION | BATCH_AUCTION
Expand Down
24 changes: 23 additions & 1 deletion contracts/p1/RevenueTrader.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ contract RevenueTraderP1 is TradingP1, IRevenueTrader {
IBackingManager private backingManager;
IFurnace private furnace;
IRToken private rToken;
IERC20 private rsr;

function init(
IMain main_,
Expand All @@ -43,6 +44,7 @@ contract RevenueTraderP1 is TradingP1, IRevenueTrader {
backingManager = main.backingManager();
furnace = main.furnace();
rToken = main.rToken();
rsr = main.rsr();
}

/// Settle a single trade + distribute revenue
Expand All @@ -62,6 +64,26 @@ contract RevenueTraderP1 is TradingP1, IRevenueTrader {
_distributeTokenToBuy();
}

/// Return registered ERC20s to the BackingManager if distribution for tokenToBuy is 0
/// @custom:interaction
function returnTokens(IERC20[] memory erc20s) external notTradingPausedOrFrozen {
RevenueTotals memory revTotals = distributor.totals();
if (tokenToBuy == rsr) {
require(revTotals.rsrTotal == 0, "rsrTotal > 0");
} else if (address(tokenToBuy) == address(rToken)) {
require(revTotals.rTokenTotal == 0, "rTokenTotal > 0");
} else {
revert("invalid tokenToBuy");
}

// Return ERC20s to the BackingManager
uint256 len = erc20s.length;
for (uint256 i = 0; i < len; ++i) {
require(assetRegistry.isRegistered(erc20s[i]), "unregistered erc20");
erc20s[i].safeTransfer(address(backingManager), erc20s[i].balanceOf(address(this)));
}
}

/// Process some number of tokens
/// If the tokenToBuy is included in erc20s, RevenueTrader will distribute it at end of the tx
/// @param erc20s The ERC20s to manage; can be tokenToBuy or anything registered
Expand Down Expand Up @@ -164,5 +186,5 @@ contract RevenueTraderP1 is TradingP1, IRevenueTrader {
* variables without shifting down storage in the inheritance chain.
* See https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps
*/
uint256[44] private __gap;
uint256[43] private __gap;
}
24 changes: 23 additions & 1 deletion contracts/plugins/mocks/InvalidRevTraderP1Mock.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ contract RevenueTraderP1InvalidReverts is TradingP1, IRevenueTrader {
IBackingManager private backingManager;
IFurnace private furnace;
IRToken private rToken;
IERC20 private rsr;

function init(
IMain main_,
Expand All @@ -35,13 +36,33 @@ contract RevenueTraderP1InvalidReverts is TradingP1, IRevenueTrader {
}

/// Distribute tokenToBuy to its destinations
function distributeTokenToBuy() public {
function distributeTokenToBuy() public notTradingPausedOrFrozen {
uint256 bal = tokenToBuy.balanceOf(address(this));
tokenToBuy.safeApprove(address(main.distributor()), 0);
tokenToBuy.safeApprove(address(main.distributor()), bal);
main.distributor().distribute(tokenToBuy, bal);
}

/// Return registered ERC20s to the BackingManager if distribution for tokenToBuy is 0
/// @custom:interaction
function returnTokens(IERC20[] memory erc20s) external notTradingPausedOrFrozen {
RevenueTotals memory revTotals = distributor.totals();
if (tokenToBuy == rsr) {
require(revTotals.rsrTotal == 0, "rsrTotal > 0");
} else if (address(tokenToBuy) == address(rToken)) {
require(revTotals.rTokenTotal == 0, "rTokenTotal > 0");
} else {
revert("invalid tokenToBuy");
}

// Return ERC20s to the BackingManager
uint256 len = erc20s.length;
for (uint256 i = 0; i < len; ++i) {
require(assetRegistry.isRegistered(erc20s[i]), "erc20 unregistered");
erc20s[i].safeTransfer(address(backingManager), erc20s[i].balanceOf(address(this)));
}
}

/// Processes a single token; unpermissioned
/// Reverts for testing purposes
function manageTokens(IERC20[] memory, TradeKind[] memory) external notTradingPausedOrFrozen {
Expand All @@ -55,5 +76,6 @@ contract RevenueTraderP1InvalidReverts is TradingP1, IRevenueTrader {
backingManager = main.backingManager();
furnace = main.furnace();
rToken = main.rToken();
rsr = main.rsr();
}
}
108 changes: 108 additions & 0 deletions test/Revenues.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -636,6 +636,114 @@ describe(`Revenues - P${IMPLEMENTATION}`, () => {
expect(await rsr.balanceOf(stRSR.address)).to.be.closeTo(expectedAmount, 100)
})

it('Should return tokens to BackingManager correctly - rsrTrader.returnTokens()', async () => {
// Mint tokens
await rsr.connect(owner).mint(rsrTrader.address, issueAmount)
await token0.connect(owner).mint(rsrTrader.address, issueAmount.add(1))
await token1.connect(owner).mint(rsrTrader.address, issueAmount.add(2))

// Should fail when trading paused or frozen
await main.connect(owner).pauseIssuance()
await main.connect(owner).pauseTrading()
await main.connect(owner).freezeForever()
await expect(
rsrTrader.returnTokens([rsr.address, token0.address, token1.address])
).to.be.revertedWith('frozen or trading paused')
await main.connect(owner).unfreeze()
await expect(
rsrTrader.returnTokens([rsr.address, token0.address, token1.address])
).to.be.revertedWith('frozen or trading paused')
await main.connect(owner).unpauseTrading()

// Should fail when distribution is nonzero
await expect(
rsrTrader.returnTokens([rsr.address, token0.address, token1.address])
).to.be.revertedWith('rsrTotal > 0')
await distributor.setDistribution(STRSR_DEST, { rTokenDist: bn('0'), rsrDist: bn('0') })

// Should fail for unregistered token
await assetRegistry.connect(owner).unregister(collateral1.address)
await expect(
rsrTrader.returnTokens([rsr.address, token0.address, token1.address])
).to.be.revertedWith('unregistered erc20')

// Succeed on just token0 + rsr
await expectEvents(rsrTrader.returnTokens([rsr.address, token0.address]), [
{
contract: rsr,
name: 'Transfer',
args: [rsrTrader.address, backingManager.address, issueAmount],
emitted: true,
},
{
contract: token0,
name: 'Transfer',
args: [rsrTrader.address, backingManager.address, issueAmount.add(1)],
emitted: true,
},
{
contract: token1,
Copy link
Collaborator

Choose a reason for hiding this comment

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

i dont think this should fire?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed, it has emitted: false to check for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

We dont need the args param if we are checking for emitted = false, maybe thats the confusion.

name: 'Transfer',
args: [anyValue, anyValue, anyValue],
emitted: false,
},
])
})

it('Should return tokens to BackingManager correctly - rTokenTrader.returnTokens()', async () => {
// Mint tokens
await rsr.connect(owner).mint(rTokenTrader.address, issueAmount)
await token0.connect(owner).mint(rTokenTrader.address, issueAmount.add(1))
await token1.connect(owner).mint(rTokenTrader.address, issueAmount.add(2))

// Should fail when trading paused or frozen
await main.connect(owner).pauseIssuance()
await main.connect(owner).pauseTrading()
await main.connect(owner).freezeForever()
await expect(
rTokenTrader.returnTokens([rsr.address, token0.address, token1.address])
).to.be.revertedWith('frozen or trading paused')
await main.connect(owner).unfreeze()
await expect(
rTokenTrader.returnTokens([rsr.address, token0.address, token1.address])
).to.be.revertedWith('frozen or trading paused')
await main.connect(owner).unpauseTrading()

// Should fail when distribution is nonzero
await expect(
rTokenTrader.returnTokens([rsr.address, token0.address, token1.address])
).to.be.revertedWith('rTokenTotal > 0')
await distributor.setDistribution(FURNACE_DEST, { rTokenDist: bn('0'), rsrDist: bn('0') })

// Should fail for unregistered token
await assetRegistry.connect(owner).unregister(collateral1.address)
await expect(
rTokenTrader.returnTokens([rsr.address, token0.address, token1.address])
).to.be.revertedWith('unregistered erc20')

// Succeed on just token0 + rsr
await expectEvents(rTokenTrader.returnTokens([rsr.address, token0.address]), [
{
contract: rsr,
name: 'Transfer',
args: [rTokenTrader.address, backingManager.address, issueAmount],
emitted: true,
},
{
contract: token0,
name: 'Transfer',
args: [rTokenTrader.address, backingManager.address, issueAmount.add(1)],
emitted: true,
},
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

i don't think this should fire?

contract: token1,
name: 'Transfer',
args: [anyValue, anyValue, anyValue],
emitted: false,
},
])
})

it('Should launch multiple auctions -- has tokenToBuy', async () => {
// Mint AAVE, token0, and RSR to the RSRTrader
await aaveToken.connect(owner).mint(rsrTrader.address, issueAmount)
Expand Down