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

[CodeArena] - [G-31] : Remove token variable from struct Holding #80

Merged
merged 2 commits into from
Jan 5, 2022
Merged

[CodeArena] - [G-31] : Remove token variable from struct Holding #80

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9ee6af1
fix: remove token variable in Holding
maximebrugel Dec 22, 2021
c924109
fix: remove Holding struct
maximebrugel Jan 3, 2022
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
2 changes: 1 addition & 1 deletion contracts/NestedBuybacker.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ contract NestedBuybacker is Ownable {
feeSplitter = FeeSplitter(_feeSplitter);
nstReserve = _nstReserve;
}

/// @notice Update the nested reserve address
/// @param _nstReserve New reserve contract address
function setNestedReserve(address _nstReserve) external onlyOwner {
Expand Down
32 changes: 18 additions & 14 deletions contracts/NestedFactory.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,10 +212,10 @@ contract NestedFactory is INestedFactory, ReentrancyGuard, Ownable, MixinOperato
uint256 buyTokenInitialBalance = _buyToken.balanceOf(address(this));

for (uint256 i = 0; i < tokensLength; i++) {
NestedRecords.Holding memory holding = nestedRecords.getAssetHolding(_nftId, tokens[i]);
reserve.withdraw(IERC20(holding.token), holding.amount);
uint256 amount = nestedRecords.getAssetHolding(_nftId, tokens[i]);
reserve.withdraw(IERC20(tokens[i]), amount);

_safeSubmitOrder(tokens[i], address(_buyToken), holding.amount, _nftId, _orders[i]);
_safeSubmitOrder(tokens[i], address(_buyToken), amount, _nftId, _orders[i]);
nestedRecords.freeHolding(_nftId, tokens[i]);
}

Expand Down Expand Up @@ -247,12 +247,11 @@ contract NestedFactory is INestedFactory, ReentrancyGuard, Ownable, MixinOperato
require(assetTokensLength > 1, "NF: UNALLOWED_EMPTY_PORTFOLIO");
require(nestedRecords.getAssetReserve(_nftId) == address(reserve), "NF: RESERVE_MISMATCH");

NestedRecords.Holding memory holding = nestedRecords.getAssetHolding(
_nftId,
nestedRecords.getAssetTokens(_nftId)[_tokenIndex]
);
reserve.withdraw(IERC20(holding.token), holding.amount);
_safeTransferWithFees(IERC20(holding.token), holding.amount, _msgSender(), _nftId);
address token = nestedRecords.getAssetTokens(_nftId)[_tokenIndex];

uint256 amount = nestedRecords.getAssetHolding(_nftId, token);
reserve.withdraw(IERC20(token), amount);
_safeTransferWithFees(IERC20(token), amount, _msgSender(), _nftId);

nestedRecords.deleteAsset(_nftId, _tokenIndex);

Expand Down Expand Up @@ -449,11 +448,13 @@ contract NestedFactory is INestedFactory, ReentrancyGuard, Ownable, MixinOperato
bool _fromReserve
) private returns (IERC20 tokenUsed) {
if (_fromReserve) {
NestedRecords.Holding memory holding = nestedRecords.getAssetHolding(_nftId, address(_inputToken));
require(holding.amount >= _inputTokenAmount, "NF: INSUFFICIENT_AMOUNT_IN");
require(
nestedRecords.getAssetHolding(_nftId, address(_inputToken)) >= _inputTokenAmount,
"NF: INSUFFICIENT_AMOUNT_IN"
);

// Get input from reserve
reserve.withdraw(IERC20(holding.token), _inputTokenAmount);
reserve.withdraw(IERC20(_inputToken), _inputTokenAmount);
} else if (address(_inputToken) == ETH) {
require(msg.value == _inputTokenAmount, "NF: INVALID_AMOUNT_IN");
weth.deposit{ value: msg.value }();
Expand Down Expand Up @@ -507,8 +508,11 @@ contract NestedFactory is INestedFactory, ReentrancyGuard, Ownable, MixinOperato
address _inputToken,
uint256 _amount
) private {
NestedRecords.Holding memory holding = nestedRecords.getAssetHolding(_nftId, _inputToken);
nestedRecords.updateHoldingAmount(_nftId, _inputToken, holding.amount - _amount);
nestedRecords.updateHoldingAmount(
_nftId,
_inputToken,
nestedRecords.getAssetHolding(_nftId, _inputToken) - _amount
);
}

/// @dev Transfer a token amount from the factory to the recipient.
Expand Down
30 changes: 11 additions & 19 deletions contracts/NestedRecords.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,11 @@ contract NestedRecords is OwnableFactoryHandler {
/// @dev Emitted when the reserve is updated for a specific portfolio
/// @param nftId The NFT ID
/// @param newReserve The new reserve address
event reserveUpdated(uint256 nftId, address newReserve);

/// @dev Info about assets stored in reserves
struct Holding {
uint256 amount;
address token;
bool isActive;
}
event reserveUpdated(uint256 nftId, address newReserve);

/// @dev Store user asset informations
struct NftRecord {
mapping(address => Holding) holdings;
mapping(address => uint256) holdings;
address[] tokens;
address reserve;
uint256 lockTimestamp;
Expand Down Expand Up @@ -65,7 +58,7 @@ contract NestedRecords is OwnableFactoryHandler {
tokenIndex++;
}
} else {
records[_nftId].holdings[_token].amount = _amount;
records[_nftId].holdings[_token] = _amount;
}
}

Expand Down Expand Up @@ -93,10 +86,10 @@ contract NestedRecords is OwnableFactoryHandler {
uint256 _amount,
address _reserve
) external onlyFactory {
Holding memory holding = records[_nftId].holdings[_token];
if (holding.isActive) {
uint256 amount = records[_nftId].holdings[_token];
if (amount != 0) {
require(records[_nftId].reserve == _reserve, "NRC: RESERVE_MISMATCH");
updateHoldingAmount(_nftId, _token, holding.amount + _amount);
updateHoldingAmount(_nftId, _token, amount + _amount);
return;
}
require(records[_nftId].tokens.length < maxHoldingsCount, "NRC: TOO_MANY_TOKENS");
Expand All @@ -105,15 +98,15 @@ contract NestedRecords is OwnableFactoryHandler {
"NRC: INVALID_RESERVE"
);

records[_nftId].holdings[_token] = Holding({ token: _token, amount: _amount, isActive: true });
records[_nftId].holdings[_token] = _amount;
records[_nftId].tokens.push(_token);
records[_nftId].reserve = _reserve;
}

/// @notice Get holding object for this NFT ID
/// @notice Get holding amount for this NFT ID
/// @param _nftId The id of the NFT
/// @param _token The address of the token
function getAssetHolding(uint256 _nftId, address _token) external view returns (Holding memory) {
function getAssetHolding(uint256 _nftId, address _token) external view returns (uint256) {
return records[_nftId].holdings[_token];
}

Expand Down Expand Up @@ -177,12 +170,11 @@ contract NestedRecords is OwnableFactoryHandler {
function deleteAsset(uint256 _nftId, uint256 _tokenIndex) public onlyFactory {
address[] storage tokens = records[_nftId].tokens;
address token = tokens[_tokenIndex];
Holding memory holding = records[_nftId].holdings[token];

require(holding.isActive, "NRC: HOLDING_INACTIVE");
require(records[_nftId].holdings[token] != 0, "NRC: HOLDING_INACTIVE");

delete records[_nftId].holdings[token];
tokens[_tokenIndex] = tokens[tokens.length - 1];
tokens.pop();
}
}
}
5 changes: 0 additions & 5 deletions test/helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,6 @@ export const getETHSpentOnGas = async (tx: any) => {
return receipt.gasUsed.mul(tx.gasPrice);
};

export const displayHoldings = (holdings: any[]) => {
console.log("Holdings: ");
holdings.forEach(holding => console.log(holding.token + " " + ethers.utils.formatEther(holding.amount)));
};

export const getTokenName = (address: string, tokens: Record<string, string>) =>
Object.entries(tokens).find(([_, value]) => value === address)?.[0] || "???";

Expand Down
5 changes: 2 additions & 3 deletions test/unit/FlatOperator.unit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,9 +118,8 @@ describe("FlatOperator", () => {
);

// Must have the right amount in the holdings
const holdingsUNI = await context.nestedRecords.getAssetHolding(1, context.mockUNI.address);
expect(holdingsUNI.token).to.be.equal(context.mockUNI.address);
expect(holdingsUNI.amount).to.be.equal(uniBought);
const holdingsUNIAmount = await context.nestedRecords.getAssetHolding(1, context.mockUNI.address);
expect(holdingsUNIAmount).to.be.equal(uniBought);
});

it("remove token from portfolio when destroy()", async () => {
Expand Down
87 changes: 35 additions & 52 deletions test/unit/NestedFactory.unit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,7 @@ describe("NestedFactory", () => {
await expect(
context.nestedFactory
.connect(context.user1)
.create(0, context.mockDAI.address, totalToSpend, orders, {value: 1}),
.create(0, context.mockDAI.address, totalToSpend, orders, { value: 1 }),
).to.be.revertedWith("NF: UNSUPPORTED_ETH_TRANSFER");
});

Expand Down Expand Up @@ -375,12 +375,10 @@ describe("NestedFactory", () => {
);

// Must have the right amount in the holdings
const holdingsUNI = await context.nestedRecords.getAssetHolding(1, context.mockUNI.address);
expect(holdingsUNI.token).to.be.equal(context.mockUNI.address);
expect(holdingsUNI.amount).to.be.equal(uniBought);
const holdingsKNC = await context.nestedRecords.getAssetHolding(1, context.mockKNC.address);
expect(holdingsKNC.token).to.be.equal(context.mockKNC.address);
expect(holdingsKNC.amount).to.be.equal(kncBought);
const holdingsUNIAmount = await context.nestedRecords.getAssetHolding(1, context.mockUNI.address);
expect(holdingsUNIAmount).to.be.equal(uniBought);
const holdingsKNCAmount = await context.nestedRecords.getAssetHolding(1, context.mockKNC.address);
expect(holdingsKNCAmount).to.be.equal(kncBought);
});

it("Creates NFT from DAI with KNI and UNI inside (ZeroExOperator) with more than needed", async () => {
Expand Down Expand Up @@ -732,12 +730,10 @@ describe("NestedFactory", () => {
);

// Must have the right amount in the holdings
const holdingsUNI = await context.nestedRecords.getAssetHolding(1, context.mockUNI.address);
expect(holdingsUNI.token).to.be.equal(context.mockUNI.address);
expect(holdingsUNI.amount).to.be.equal(uniBought.add(baseUniBought));
const holdingsKNC = await context.nestedRecords.getAssetHolding(1, context.mockKNC.address);
expect(holdingsKNC.token).to.be.equal(context.mockKNC.address);
expect(holdingsKNC.amount).to.be.equal(kncBought.add(baseKncBought));
const holdingsUNIAmount = await context.nestedRecords.getAssetHolding(1, context.mockUNI.address);
expect(holdingsUNIAmount).to.be.equal(uniBought.add(baseUniBought));
const holdingsKNCAmount = await context.nestedRecords.getAssetHolding(1, context.mockKNC.address);
expect(holdingsKNCAmount).to.be.equal(kncBought.add(baseKncBought));
});

it("increase KNI and UNI amount from DAI (ZeroExOperator) with more than needed", async () => {
Expand Down Expand Up @@ -827,15 +823,12 @@ describe("NestedFactory", () => {
);

// Must have the right amount in the holdings
const holdingsUNI = await context.nestedRecords.getAssetHolding(1, context.mockUNI.address);
expect(holdingsUNI.token).to.be.equal(context.mockUNI.address);
expect(holdingsUNI.amount).to.be.equal(baseUniBought);
const holdingsKNC = await context.nestedRecords.getAssetHolding(1, context.mockKNC.address);
expect(holdingsKNC.token).to.be.equal(context.mockKNC.address);
expect(holdingsKNC.amount).to.be.equal(baseKncBought);
const holdingsDAI = await context.nestedRecords.getAssetHolding(1, context.mockDAI.address);
expect(holdingsDAI.token).to.be.equal(context.mockDAI.address);
expect(holdingsDAI.amount).to.be.equal(daiBought);
const holdingsUNIAmount = await context.nestedRecords.getAssetHolding(1, context.mockUNI.address);
expect(holdingsUNIAmount).to.be.equal(baseUniBought);
const holdingsKNCAmount = await context.nestedRecords.getAssetHolding(1, context.mockKNC.address);
expect(holdingsKNCAmount).to.be.equal(baseKncBought);
const holdingsDAIAmount = await context.nestedRecords.getAssetHolding(1, context.mockDAI.address);
expect(holdingsDAIAmount).to.be.equal(daiBought);
});
});

Expand Down Expand Up @@ -1023,9 +1016,8 @@ describe("NestedFactory", () => {
);

// Must have the right amount in the holdings
const holdingsUNI = await context.nestedRecords.getAssetHolding(1, context.mockUNI.address);
expect(holdingsUNI.token).to.be.equal(context.mockUNI.address);
expect(holdingsUNI.amount).to.be.equal(uniBought.add(baseUniBought));
const holdingsUNIAmount = await context.nestedRecords.getAssetHolding(1, context.mockUNI.address);
expect(holdingsUNIAmount).to.be.equal(uniBought.add(baseUniBought));
});

it("increase UNI amount from KNC in portfolio (ZeroExOperator) with more than needed", async () => {
Expand Down Expand Up @@ -1099,15 +1091,12 @@ describe("NestedFactory", () => {
);

// Must have the right amount in the holdings
const holdingsUNI = await context.nestedRecords.getAssetHolding(1, context.mockUNI.address);
expect(holdingsUNI.token).to.be.equal(context.mockUNI.address);
expect(holdingsUNI.amount).to.be.equal(baseUniBought.sub(totalToSpend));
const holdingsKNC = await context.nestedRecords.getAssetHolding(1, context.mockKNC.address);
expect(holdingsKNC.token).to.be.equal(context.mockKNC.address);
expect(holdingsKNC.amount).to.be.equal(baseKncBought);
const holdingsUSDC = await context.nestedRecords.getAssetHolding(1, context.mockUSDC.address);
expect(holdingsUSDC.token).to.be.equal(context.mockUSDC.address);
expect(holdingsUSDC.amount).to.be.equal(usdcBought);
const holdingsUNIAmount = await context.nestedRecords.getAssetHolding(1, context.mockUNI.address);
expect(holdingsUNIAmount).to.be.equal(baseUniBought.sub(totalToSpend));
const holdingsKNCAmount = await context.nestedRecords.getAssetHolding(1, context.mockKNC.address);
expect(holdingsKNCAmount).to.be.equal(baseKncBought);
const holdingsUSDCAmount = await context.nestedRecords.getAssetHolding(1, context.mockUSDC.address);
expect(holdingsUSDCAmount).to.be.equal(usdcBought);
});
});

Expand Down Expand Up @@ -1291,14 +1280,12 @@ describe("NestedFactory", () => {
);

// Must have the right amount in the holdings
const holdingsUNI = await context.nestedRecords.getAssetHolding(1, context.mockUNI.address);
expect(holdingsUNI.token).to.be.equal(context.mockUNI.address);
expect(holdingsUNI.amount).to.be.equal(baseUniBought.sub(uniSold));
const holdingsUNIAmount = await context.nestedRecords.getAssetHolding(1, context.mockUNI.address);
expect(holdingsUNIAmount).to.be.equal(baseUniBought.sub(uniSold));

// Must have the right amount in the holdings
const holdingsUSDC = await context.nestedRecords.getAssetHolding(1, context.mockUSDC.address);
expect(holdingsUSDC.token).to.be.equal(context.mockUSDC.address);
expect(holdingsUSDC.amount).to.be.equal(usdcBought.sub(expectedUsdcFees));
const holdingsUSDCAmount = await context.nestedRecords.getAssetHolding(1, context.mockUSDC.address);
expect(holdingsUSDCAmount).to.be.equal(usdcBought.sub(expectedUsdcFees));
});

it("swap KNC and UNI for USDC (ZeroExOperator) with more than needed", async () => {
Expand Down Expand Up @@ -1376,13 +1363,11 @@ describe("NestedFactory", () => {
);

// Must have the right amount in the holdings
const holdingsUNI = await context.nestedRecords.getAssetHolding(1, context.mockUNI.address);
expect(holdingsUNI.token).to.be.equal(context.mockUNI.address);
expect(holdingsUNI.amount).to.be.equal(baseUniBought.sub(uniSold));
const holdingsUNIAmount = await context.nestedRecords.getAssetHolding(1, context.mockUNI.address);
expect(holdingsUNIAmount).to.be.equal(baseUniBought.sub(uniSold));

const holdingsUSDC = await context.nestedRecords.getAssetHolding(1, context.mockUSDC.address);
expect(holdingsUSDC.token).to.be.equal(context.mockUSDC.address);
expect(holdingsUSDC.amount).to.be.equal(usdcBoughtOrder.sub(orderExpectedFee));
const holdingsUSDCAmount = await context.nestedRecords.getAssetHolding(1, context.mockUSDC.address);
expect(holdingsUSDCAmount).to.be.equal(usdcBoughtOrder.sub(orderExpectedFee));
});
});

Expand Down Expand Up @@ -1565,9 +1550,8 @@ describe("NestedFactory", () => {
);

// Must have the right amount in the holdings
const holdingsUNI = await context.nestedRecords.getAssetHolding(1, context.mockUNI.address);
expect(holdingsUNI.token).to.be.equal(context.mockUNI.address);
expect(holdingsUNI.amount).to.be.equal(baseUniBought.sub(uniSold));
const holdingsUNIAmount = await context.nestedRecords.getAssetHolding(1, context.mockUNI.address);
expect(holdingsUNIAmount).to.be.equal(baseUniBought.sub(uniSold));

expect(await context.mockUSDC.balanceOf(context.user1.address)).to.be.equal(
context.baseAmount.add(usdcBought.sub(expectedUsdcFees)),
Expand Down Expand Up @@ -1646,9 +1630,8 @@ describe("NestedFactory", () => {
);

// Must have the right amount in the holdings
const holdingsUNI = await context.nestedRecords.getAssetHolding(1, context.mockUNI.address);
expect(holdingsUNI.token).to.be.equal(context.mockUNI.address);
expect(holdingsUNI.amount).to.be.equal(baseUniBought.sub(uniSold));
const holdingsUNIAmount = await context.nestedRecords.getAssetHolding(1, context.mockUNI.address);
expect(holdingsUNIAmount).to.be.equal(baseUniBought.sub(uniSold));
});
});

Expand Down