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 all commits
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
34 changes: 30 additions & 4 deletions contracts/StableJoeStaking.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/math/SafeMathUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/proxy/Initializable.sol";
import "@openzeppelin/contracts-upgradeable/token/ERC20/IERC20Upgradeable.sol";
import "@openzeppelin/contracts-upgradeable/token/ERC721/IERC721Upgradeable.sol";
import "@openzeppelin/contracts-upgradeable/token/ERC20/SafeERC20Upgradeable.sol";

/**
Expand Down Expand Up @@ -66,6 +67,9 @@ contract StableJoeStaking is Initializable, OwnableUpgradeable {
/// @dev Info of each user that stakes JOE
mapping(address => UserInfo) private userInfo;

/// @dev Smol Joes contract address
IERC721Upgradeable public smolJoes;

/// @notice Emitted when a user deposits JOE
event Deposit(address indexed user, uint256 amount, uint256 fee);

Expand All @@ -87,6 +91,9 @@ contract StableJoeStaking is Initializable, OwnableUpgradeable {
/// @notice Emitted when owner removes a token from the reward tokens list
event RewardTokenRemoved(address token);

/// @notice Emitted when Smol Joes address is changed
event SmolJoesInitialized(IERC721Upgradeable newSmolJoes, IERC721Upgradeable oldSmolJoes);

/**
* @notice Initialize a new StableJoeStaking contract
* @dev This contract needs to receive an ERC20 `_rewardToken` in order to distribute them
Expand All @@ -100,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 @@ -125,7 +135,11 @@ contract StableJoeStaking is Initializable, OwnableUpgradeable {
function deposit(uint256 _amount) external {
UserInfo storage user = userInfo[_msgSender()];

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

@jummy123 jummy123 Sep 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Tests are failing as you are comparing address with int. 0 needs to be cast to address address(0) (I also think I made the same error when proposing the code snippet)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think there is a potential for a problem here if you deploy this update and don't set the smolJoe variable immediately, as && short circuits nobody would pay any fee on deposit until smolJoe address was set. It looks like this will be covered by the deployment script but still worth being aware.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the following fixes the above problem.

if (!(address(smolJoes) != address(0) && smolJoes.balanceOf(_msgSender()) > 0 && _msgSender() == tx.origin)) {
     _fee = _amount.mul(depositFeePercent).div(DEPOSIT_FEE_PERCENT_PRECISION);
}

_fee = _amount.mul(depositFeePercent).div(DEPOSIT_FEE_PERCENT_PRECISION);
}
uint256 _amountMinusFee = _amount.sub(_fee);

uint256 _previousAmount = user.amount;
Expand Down Expand Up @@ -153,8 +167,8 @@ contract StableJoeStaking is Initializable, OwnableUpgradeable {
}

internalJoeBalance = internalJoeBalance.add(_amountMinusFee);
joe.safeTransferFrom(_msgSender(), feeCollector, _fee);
joe.safeTransferFrom(_msgSender(), address(this), _amountMinusFee);
if (_fee > 0) joe.safeTransferFrom(_msgSender(), feeCollector, _fee);
if (_amountMinusFee > 0) joe.safeTransferFrom(_msgSender(), address(this), _amountMinusFee);
emit Deposit(_msgSender(), _amountMinusFee, _fee);
}

Expand Down Expand Up @@ -224,6 +238,18 @@ contract StableJoeStaking is Initializable, OwnableUpgradeable {
emit DepositFeeChanged(_depositFeePercent, oldFee);
}

/**
* @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 setSmolJoes(IERC721Upgradeable _smolJoes) external onlyOwner {
require(address(_smolJoes) != address(0), "StableJoeStaking: smol joes can't be address(0)");
smolJoes = _smolJoes;
emit SmolJoesInitialized(_smolJoes, smolJoes);
}

/**
* @notice View function to see pending reward token on frontend
* @param _user The address of the user
Expand Down
16 changes: 16 additions & 0 deletions contracts/mocks/ERC721Mock.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// SPDX-License-Identifier: MIT

pragma solidity 0.6.12;

import "@openzeppelin/contracts/token/ERC721/ERC721.sol";

contract ERC721Mock is ERC721 {
uint256 tokenOwners;

constructor(string memory name, string memory symbol) public ERC721(name, symbol) {}

function mint(address _account) external {
_mint(_account, tokenOwners);
tokenOwners += 1;
}
}
22 changes: 22 additions & 0 deletions contracts/mocks/StableJoeVaultMock.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// SPDX-License-Identifier: MIT

pragma solidity 0.7.6;

import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "../StableJoeStaking.sol";

contract StableJoeVaultMock {
IERC20 public immutable joe;
StableJoeStaking public immutable sJoe;

constructor(IERC20 _joe, StableJoeStaking _sJoe) public {
joe = _joe;
sJoe = _sJoe;
}

function deposit(uint256 _amount) external {
joe.transferFrom(msg.sender, address(this), _amount);
joe.approve(address(sJoe), _amount);
sJoe.deposit(_amount);
}
}
43 changes: 36 additions & 7 deletions deploy/StableJoeStaking.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,12 @@ module.exports = async function ({ getNamedAccounts, deployments }) {
const { deploy, catchUnknownSigner } = deployments;
const { deployer } = await getNamedAccounts();

let rewardToken, joeAddress, feeCollector, depositFeePercent, proxyOwner;
let rewardToken,
joeAddress,
feeCollector,
depositFeePercent,
smolJoes,
proxyOwner;

const chainId = await getChainId();
if (chainId == 4) {
Expand All @@ -12,33 +17,57 @@ module.exports = async function ({ getNamedAccounts, deployments }) {
rewardToken = "0x9Ad6C38BE94206cA50bb0d90783181662f0Cfa10";
feeCollector = "0x2fbB61a10B96254900C03F1644E9e1d2f5E76DD2";
depositFeePercent = 0;
proxyOwner = deployer.address;
smolJoes = "0xce347E069B68C53A9ED5e7DA5952529cAF8ACCd4"; // use an ERC20 as placeholder for now
proxyOwner = deployer;
} else if (chainId == 43114 || chainId == 31337) {
// avalanche mainnet or hardhat network addresses
joeAddress = "0x6e84a6216eA6dACC71eE8E6b0a5B7322EEbC0fDd";
// USDC.e
rewardToken = "0xB97EF9Ef8734C71904D8002F8b6Bc66Dd9c48a6E";
feeCollector = "0x2fbB61a10B96254900C03F1644E9e1d2f5E76DD2";
depositFeePercent = 0;
smolJoes = "0xC70DF87e1d98f6A531c8E324C9BCEC6FC82B5E8d";
proxyOwner = "0x2fbB61a10B96254900C03F1644E9e1d2f5E76DD2";
}

await catchUnknownSigner(
deploy("StableJoeStaking", {
await catchUnknownSigner(async () => {
const sJoe = await deploy("StableJoeStaking", {
from: deployer,
proxy: {
owner: proxyOwner,
proxyContract: "OpenZeppelinTransparentProxy",
execute: {
init: {
methodName: "initialize",
args: [rewardToken, joeAddress, feeCollector, depositFeePercent],
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 sJoeImpl = await ethers.getContract(
"StableJoeStaking_Implementation"
);
await sJoeImpl.initialize(
rewardToken,
joeAddress,
feeCollector,
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

}
});
};

module.exports.tags = ["StableJoeStaking"];
Expand Down
48 changes: 48 additions & 0 deletions test/StableJoeStaking.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,39 +9,54 @@ describe("Stable Joe Staking", function () {
this.StableJoeStakingCF = await ethers.getContractFactory(
"StableJoeStaking"
);
this.StableJoeVaultCF = await ethers.getContractFactory(
"StableJoeVaultMock"
);
this.JoeTokenCF = await ethers.getContractFactory("JoeToken");
this.SmolJoesCF = await ethers.getContractFactory("ERC721Mock");

this.signers = await ethers.getSigners();
this.dev = this.signers[0];
this.alice = this.signers[1];
this.bob = this.signers[2];
this.carol = this.signers[3];
this.dylan = this.signers[4];
this.joeMaker = this.signers[4];
this.penaltyCollector = this.signers[5];
});

beforeEach(async function () {
this.rewardToken = await this.JoeTokenCF.deploy();
this.joe = await this.JoeTokenCF.deploy();
this.smolJoes = await this.SmolJoesCF.deploy("Smol Joes", "SMOL JOES");

await this.joe.mint(this.alice.address, ethers.utils.parseEther("1000"));
await this.joe.mint(this.bob.address, ethers.utils.parseEther("1000"));
await this.joe.mint(this.carol.address, ethers.utils.parseEther("1000"));
await this.joe.mint(this.dylan.address, ethers.utils.parseEther("1000"));
await this.rewardToken.mint(
this.joeMaker.address,
ethers.utils.parseEther("1000000")
); // 1_000_000 tokens

await this.smolJoes.mint(this.dylan.address);

this.stableJoeStaking = await hre.upgrades.deployProxy(
this.StableJoeStakingCF,
[
this.rewardToken.address,
this.joe.address,
this.penaltyCollector.address,
ethers.utils.parseEther("0.03"),
this.smolJoes.address,
]
);

this.stableJoeVault = await this.StableJoeVaultCF.deploy(
this.joe.address,
this.stableJoeStaking.address
);

await this.joe
.connect(this.alice)
.approve(
Expand All @@ -60,6 +75,16 @@ describe("Stable Joe Staking", function () {
this.stableJoeStaking.address,
ethers.utils.parseEther("100000")
);
await this.joe
.connect(this.dylan)
.approve(
this.stableJoeStaking.address,
ethers.utils.parseEther("100000")
);

await this.joe
.connect(this.dylan)
.approve(this.stableJoeVault.address, ethers.utils.parseEther("100000"));
});

describe("should allow deposits and withdraws", function () {
Expand Down Expand Up @@ -672,6 +697,29 @@ describe("Stable Joe Staking", function () {
);
});

it("should allow EOAs with Smol Joes to be exempt from paying deposit fee", async function () {
await this.stableJoeStaking
.connect(this.dylan)
.deposit(ethers.utils.parseEther("100"));
Copy link
Contributor

@jummy123 jummy123 Nov 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

There doesn't look to be a bug there but for fuller test coverage you should also deposit via the vault before transferring the ERC721 to it.

expect(
await this.joe.balanceOf(this.stableJoeStaking.address)
).to.be.equal(ethers.utils.parseEther("100"));

// Transfer Smol Joe to vault contract and deposit via vault
await this.smolJoes
.connect(this.dylan)
.transferFrom(this.dylan.address, this.stableJoeVault.address, 0);
await this.stableJoeVault
.connect(this.dylan)
.deposit(ethers.utils.parseEther("100"));
expect(
await this.joe.balanceOf(this.stableJoeStaking.address)
).to.be.equal(ethers.utils.parseEther("197"));
expect(
await this.joe.balanceOf(this.penaltyCollector.address)
).to.be.equal(ethers.utils.parseEther("3"));
});

it("should allow emergency withdraw", async function () {
await this.stableJoeStaking
.connect(this.alice)
Expand Down