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

No deposit fee #117

Closed
wants to merge 10 commits into from
Closed

No deposit fee #117

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
ee4426b
Add deposit fee exemption for EOAs with Smol Joes
cryptofish7 Sep 2, 2022
80e9db9
Add test for deposit fee exemption for EOAs with Smol Joes
cryptofish7 Sep 2, 2022
e68cd09
Modify deploy script for sJOE
cryptofish7 Sep 2, 2022
b9048ef
Add Louis' nits
cryptofish7 Sep 3, 2022
1d5a73b
Init implementation
cryptofish7 Sep 3, 2022
8df35d7
Change setter to initializer
cryptofish7 Sep 3, 2022
b005318
Change init function on deploy script
cryptofish7 Sep 3, 2022
a468913
Add fixes
cryptofish7 Sep 13, 2022
138c840
Fix conditional
cryptofish7 Sep 15, 2022
fd88471
Make smolJoes var public
cryptofish7 Sep 15, 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
14 changes: 9 additions & 5 deletions contracts/StableJoeStaking.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,15 +107,18 @@ contract StableJoeStaking is Initializable, OwnableUpgradeable {
IERC20Upgradeable _rewardToken,
IERC20Upgradeable _joe,
address _feeCollector,
uint256 _depositFeePercent
uint256 _depositFeePercent,
IERC721Upgradeable _smolJoes
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a setter for smolJoe's address as we can't use initialize during an upgrade as the contract has already been initialized.

) external initializer {
__Ownable_init();
require(address(_rewardToken) != address(0), "StableJoeStaking: reward token can't be address(0)");
require(address(_joe) != address(0), "StableJoeStaking: joe can't be address(0)");
require(address(_smolJoes) != address(0), "StableJoeStaking: smol joes can't be address(0)");
require(_feeCollector != address(0), "StableJoeStaking: fee collector can't be address(0)");
require(_depositFeePercent <= 5e17, "StableJoeStaking: max deposit fee can't be greater than 50%");

joe = _joe;
smolJoes = _smolJoes;
depositFeePercent = _depositFeePercent;
feeCollector = _feeCollector;

Expand All @@ -134,7 +137,7 @@ contract StableJoeStaking is Initializable, OwnableUpgradeable {

uint256 _fee;
// Only EOAs holding Smol Joes are exempt from paying the deposit fee
if (smolJoes.balanceOf(_msgSender()) == 0 || _msgSender() != tx.origin) {
if ((address(smolJoes) != 0 && smolJoes.balanceOf(_msgSender()) == 0) || _msgSender() != tx.origin) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrong parenthesis, if smolJoe is not set EOAs wouldn't pay any fees, change to:
if (address(smolJoes) != 0 && (smolJoes.balanceOf(_msgSender()) == 0 || _msgSender() != tx.origin)) {

_fee = _amount.mul(depositFeePercent).div(DEPOSIT_FEE_PERCENT_PRECISION);
}
uint256 _amountMinusFee = _amount.sub(_fee);
Expand Down Expand Up @@ -237,11 +240,12 @@ contract StableJoeStaking is Initializable, OwnableUpgradeable {

/**
* @notice Initialize the Smol Joes address
* @dev This function was added to be able to set Smol Joes during an upgrade
* as the contract was already initialized
* @param _smolJoes The Smol Joes contract address
*/
function initializeSmolJoes(IERC721Upgradeable _smolJoes) external onlyOwner {
require(address(_smolJoes) != address(0), "StableJoeStaking: smol joes can't be address(0)");
require(address(smolJoes) == address(0), "StableJoeStaking: smol joes already initialized");
function setSmolJoes(IERC721Upgradeable _smolJoes) external onlyOwner {
require(address(_smolJoes) != address(0), "StableJoeStaking: smol joes can't be address(0)3");
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo address(0)3

smolJoes = _smolJoes;
emit SmolJoesInitialized(_smolJoes, smolJoes);
}
Expand Down
22 changes: 17 additions & 5 deletions deploy/StableJoeStaking.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,22 +38,34 @@ module.exports = async function ({ getNamedAccounts, deployments }) {
proxyContract: "OpenZeppelinTransparentProxy",
execute: {
init: {
methodName: "initializeSmolJoes",
args: [smolJoes],
methodName: "initialize",
args: [
rewardToken,
joeAddress,
feeCollector,
depositFeePercent,
smolJoes,
],
},
},
},
Copy link
Contributor

@0x0Louis 0x0Louis Sep 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

For safety reasons, don't forget to initialize the implementation even if it's useless for transparent proxies.

log: true,
});
if (sJoe.newlyDeployed) {
console.log("Initializing implementation for safe measure...");
const impl = await ethers.getContract("StableJoeStaking_Implementation");
await impl.initialize(
const sJoeImpl = await ethers.getContract(
"StableJoeStaking_Implementation"
);
await sJoeImpl.initialize(
rewardToken,
joeAddress,
feeCollector,
depositFeePercent
depositFeePercent,
smolJoes
);
console.log("Setting Smol Joes...");
const sJoeProxy = await ethers.getContract("StableJoeStaking");
await sJoeProxy.setSmolJoes(smolJoes);
Comment on lines +66 to +68
Copy link
Contributor

Choose a reason for hiding this comment

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

If it's newly deployed then this is not necessary, as it would set it with initialize

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On the upgrade though, it will be "newly deployed" but it won't call initialize.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh indeed, my bad I thought it was setting it on the implementation again

}
});
};
Expand Down