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.

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

Staking #52

Merged
merged 65 commits into from
Dec 7, 2023
Merged
Show file tree
Hide file tree
Changes from 28 commits
Commits
Show all changes
65 commits
Select commit Hold shift + click to select a range
05030cf
Add OpenZeppelin contracts
r-czajkowski Nov 24, 2023
c80b77c
Update solhint rules
r-czajkowski Nov 24, 2023
3ebb88b
Initial implementation of the main Acre contract
r-czajkowski Nov 24, 2023
c81cfd5
Add initial implementation of the stake function.
r-czajkowski Nov 24, 2023
e754aca
Add `@thesis/solidity-contracts` dependency
r-czajkowski Nov 27, 2023
f36c65c
Add support for receive approval pattern to Acre
r-czajkowski Nov 27, 2023
14c14e1
Add test token contract
r-czajkowski Nov 27, 2023
02c2b3b
Add basic unit tests for staking
r-czajkowski Nov 27, 2023
a9b6c0c
Remove unnecessary `super` keyword
r-czajkowski Nov 28, 2023
a4f9f87
Update the name of the ERC4626 token
r-czajkowski Nov 28, 2023
028319f
Remove unnecessary undersocre for `_token`
r-czajkowski Nov 28, 2023
f1a126b
Update the `stake` function
r-czajkowski Nov 28, 2023
684d667
Update the Acre unit tests
r-czajkowski Nov 28, 2023
829c3ed
Rename param in `stake` function
r-czajkowski Nov 28, 2023
ffaaba6
Update `Acre` unit tests
r-czajkowski Nov 28, 2023
142c71f
Merge branch 'main' into token-vault
r-czajkowski Nov 28, 2023
b8a5b96
Fix typo
r-czajkowski Nov 30, 2023
6132a7b
Add more unit tests for staking flow
r-czajkowski Nov 30, 2023
3a3cb35
Add `Staked` event
r-czajkowski Nov 30, 2023
e319a62
Merge branch 'main' into token-vault
r-czajkowski Nov 30, 2023
6837e74
Revert "Add support for receive approval pattern to Acre"
r-czajkowski Nov 30, 2023
c960102
Remove unit tests for receive approval pattern
r-czajkowski Nov 30, 2023
c3c674d
Merge branch 'main' into token-vault
r-czajkowski Nov 30, 2023
58d2ddd
Rename variable in fixture
r-czajkowski Nov 30, 2023
05b2091
Update unit test for staking
r-czajkowski Nov 30, 2023
a77b1a4
Rename variable
r-czajkowski Nov 30, 2023
4c9eeec
Fix typo
r-czajkowski Nov 30, 2023
249fb11
Rename variable in tests
r-czajkowski Nov 30, 2023
20086c6
Update staking flow unit tests
r-czajkowski Nov 30, 2023
63aac1b
Use unnamed return in the `stake` fn
r-czajkowski Dec 4, 2023
e923f5e
Leave TODO in the `stake` fn
r-czajkowski Dec 4, 2023
5403ac3
Rename constructor parameter in Acre
r-czajkowski Dec 4, 2023
6511fef
Remove `@thesis/solidity-contracts` dependency
r-czajkowski Dec 4, 2023
84bdf8e
Add unit test heleprs
r-czajkowski Dec 4, 2023
ce16fba
Add more unit tests for staking
r-czajkowski Dec 4, 2023
c3a2bd8
Update solhint config
r-czajkowski Dec 4, 2023
b20259e
Add more stakers to the Acre test fixture
r-czajkowski Dec 4, 2023
300b2a9
Rename variable in Acre unit tests
r-czajkowski Dec 5, 2023
44b3f85
Update comment in Acre unit tests
r-czajkowski Dec 5, 2023
b84ee1b
Update staking flow unit tests
r-czajkowski Dec 5, 2023
3a312f7
Add documentation comments for `Acre` contract
r-czajkowski Dec 5, 2023
05fb49d
Merge branch 'main' into token-vault
r-czajkowski Dec 5, 2023
277dc1e
Rename `Staked` event
r-czajkowski Dec 5, 2023
eb91b4d
Update the `stake` fn
r-czajkowski Dec 5, 2023
fbb2085
Update `stake` fn docs
r-czajkowski Dec 5, 2023
48eeed3
Rename variable in Acre unit tests
r-czajkowski Dec 5, 2023
f7003b8
Rename variable in `acreFixture`
r-czajkowski Dec 5, 2023
076a29c
Update the `acreFixture` fn
r-czajkowski Dec 5, 2023
961580b
Update the test name
r-czajkowski Dec 5, 2023
9674f5e
Rename file
r-czajkowski Dec 5, 2023
9737ed8
Avoid infinite approvals in unit tests
r-czajkowski Dec 5, 2023
d33c552
Get rid of unnecessary `Staker` type in unit tests
r-czajkowski Dec 5, 2023
eef74bc
Hardcode expected values in unit tests
r-czajkowski Dec 5, 2023
8a243cf
Remove unnecessary check in unit test
r-czajkowski Dec 5, 2023
b812107
Rename test context name
r-czajkowski Dec 5, 2023
910a399
Update `without referral` test case
r-czajkowski Dec 5, 2023
a868dce
Improve test case
r-czajkowski Dec 5, 2023
567b52c
Make sure the tx is reverted with expected reason
r-czajkowski Dec 5, 2023
4889a03
Use dedicated variable in unit test
r-czajkowski Dec 5, 2023
9cac49c
Update staking unit tests
r-czajkowski Dec 5, 2023
88ee4d6
Complicate the math in test scenario
r-czajkowski Dec 5, 2023
2750286
Improve Acre contract docs comment
r-czajkowski Dec 6, 2023
ed73109
Remove unnecessary check expectation in test
r-czajkowski Dec 6, 2023
68b81d0
Remove unnecessary check
r-czajkowski Dec 6, 2023
e55dad2
Merge branch 'main' into token-vault
r-czajkowski Dec 6, 2023
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
4 changes: 3 additions & 1 deletion core/.solhint.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
{
"extends": "thesis",
"plugins": [],
"rules": {}
"rules": {
"func-visibility": ["off", { "ignoreConstructors": true }]
}
nkuba marked this conversation as resolved.
Show resolved Hide resolved
}
30 changes: 26 additions & 4 deletions core/contracts/Acre.sol
Original file line number Diff line number Diff line change
@@ -1,9 +1,31 @@
// SPDX-License-Identifier: GPL-3.0-only
pragma solidity ^0.8.21;

// Uncomment this line to use console.log
// import "hardhat/console.sol";
import "@openzeppelin/contracts/token/ERC20/extensions/ERC4626.sol";

contract Acre {
// TODO: add your implementation
contract Acre is ERC4626 {
dimpar marked this conversation as resolved.
Show resolved Hide resolved
event Staked(bytes32 indexed referral, uint256 assets, uint256 shares);

constructor(
IERC20 _token
nkuba marked this conversation as resolved.
Show resolved Hide resolved
) ERC4626(_token) ERC20("Acre Staked Bitcoin", "stBTC") {}

/// @notice Stakes a given amount of underlying token and mints shares to a
/// receiver.
/// @dev This function calls `deposit` function from `ERC4626` contract.
nkuba marked this conversation as resolved.
Show resolved Hide resolved
/// @param assets Approved amount for the transfer and stake.
/// @param receiver The address to which the shares will be minted.
/// @param referral Data used for referral program.
/// @return shares Minted shares.
function stake(
uint256 assets,
address receiver,
bytes32 referral
dimpar marked this conversation as resolved.
Show resolved Hide resolved
) public returns (uint256 shares) {
shares = deposit(assets, receiver);

emit Staked(referral, assets, shares);
Copy link
Member

Choose a reason for hiding this comment

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

I remember you proposed to emit this event only if the referral is not empty. After reconsidering it I think this is a good idea.

Copy link
Member

Choose a reason for hiding this comment

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

I also think it's a good idea to emit the referral only when it's present. More, I'd vote for renaming this event to e.g. RefferedBy(refferal) and put only the refferal id there. The reasons are a) we already have an event for deposit covering emit Deposit(caller, receiver, assets, shares) b) we do not clutter tx with duplicate data and we save gas by having less params in the event.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I agree and mentioned about it here #52 (comment). But I'm not sure about adding more context to this event as Kuba mentioned #52 (comment). I think we'll be ok if we keep only referral param. I like the ReferredBy name and I believe we should use this name. @nkuba do you agree?

Copy link
Member

Choose a reason for hiding this comment

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

How about this?

event StakeReferral(bytes32 indexed referral, uint256 assets);

Copy link
Contributor Author

@r-czajkowski r-czajkowski Dec 5, 2023

Choose a reason for hiding this comment

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


return shares;
dimpar marked this conversation as resolved.
Show resolved Hide resolved
}
}
31 changes: 31 additions & 0 deletions core/contracts/test/TestToken.sol
nkuba marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
// SPDX-License-Identifier: GPL-3.0-only
pragma solidity ^0.8.21;

import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "@thesis/solidity-contracts/contracts/token/IApproveAndCall.sol";
import "@thesis/solidity-contracts/contracts/token/IReceiveApproval.sol";

contract TestToken is ERC20, IApproveAndCall {
nkuba marked this conversation as resolved.
Show resolved Hide resolved
nkuba marked this conversation as resolved.
Show resolved Hide resolved
constructor() ERC20("Test Token", "TEST") {}

function mint(address account, uint256 value) external {
_mint(account, value);
}

function approveAndCall(
address spender,
uint256 amount,
bytes memory extraData
) external returns (bool) {
if (approve(spender, amount)) {
IReceiveApproval(spender).receiveApproval(
msg.sender,
amount,
address(this),
extraData
);
return true;
}
return false;
}
}
4 changes: 4 additions & 0 deletions core/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -55,5 +55,9 @@
"ts-node": "^10.9.1",
"typechain": "^8.3.2",
"typescript": "^5.3.2"
},
"dependencies": {
"@openzeppelin/contracts": "^5.0.0",
"@thesis/solidity-contracts": "github:thesis/solidity-contracts#c315b9d"
nkuba marked this conversation as resolved.
Show resolved Hide resolved
}
}
284 changes: 282 additions & 2 deletions core/test/Acre.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,285 @@
import {
SnapshotRestorer,
loadFixture,
takeSnapshot,
} from "@nomicfoundation/hardhat-toolbox/network-helpers"
import { HardhatEthersSigner } from "@nomicfoundation/hardhat-ethers/signers"
import { ethers } from "hardhat"
import { expect } from "chai"
import { WeiPerEther } from "ethers"
import type { TestToken, Acre } from "../typechain"

async function acreFixture() {
const [_, staker] = await ethers.getSigners()
const Token = await ethers.getContractFactory("TestToken")
const tbtc = await Token.deploy()

const amountToMint = WeiPerEther * 100000n
nkuba marked this conversation as resolved.
Show resolved Hide resolved

tbtc.mint(staker, amountToMint)

const Acre = await ethers.getContractFactory("Acre")
const acre = await Acre.deploy(await tbtc.getAddress())

return { acre, tbtc, staker }
}

describe("Acre", () => {
describe("Add your tests", () => {
it("Should validate something", async () => {})
let acre: Acre
let tbtc: TestToken
let staker: HardhatEthersSigner

before(async () => {
;({ acre, tbtc, staker } = await loadFixture(acreFixture))
})

describe("Staking", () => {
nkuba marked this conversation as resolved.
Show resolved Hide resolved
const referral = ethers.encodeBytes32String("referral")
let snapshot: SnapshotRestorer

context("when staking via Acre contract", () => {
Copy link
Member

Choose a reason for hiding this comment

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

Can we add more test cases:

  • when amountToStake is zero,
  • when amountToStake is one,
  • when amountToStake is greater than the approved amount,
  • when receiver is address zero,
  • when staker approved amountToStake, staked it and tries to stake again, without another approval.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when amountToStake is zero,

The ERC46626 from OZ seems to allow that. Shouldn't we add this validation to the deposit function as a general rule?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

when amountToStake is zero,

The ERC46626 from OZ seems to allow that. Shouldn't we add this validation to the deposit function as a general rule?

We should introduce a minimum deposit limit, we will add it in #59.

const amountToStake = 1000n * WeiPerEther

beforeEach(async () => {
snapshot = await takeSnapshot()

await tbtc
.connect(staker)
.approve(await acre.getAddress(), amountToStake)
})

afterEach(async () => {
await snapshot.restore()
})

it("should stake tokens and receive shares", async () => {
const stakerAddress = staker.address
const balanceOfBeforeStake = await tbtc.balanceOf(stakerAddress)

const tx = await acre
.connect(staker)
.stake(amountToStake, stakerAddress, referral)

const stakedTokens = amountToStake

// In this test case there is only one staker and
// the token vault has not earned anythig yet so received shares are
// equal to staked tokens amount.
nkuba marked this conversation as resolved.
Show resolved Hide resolved
const receivedShares = amountToStake

expect(tx).to.emit(acre, "Deposit").withArgs(
nkuba marked this conversation as resolved.
Show resolved Hide resolved
// Caller.
stakerAddress,
// Receiver.
stakerAddress,
// Staked tokens.
stakedTokens,
// Received shares.
receivedShares,
)

expect(tx)
.to.emit(acre, "Staked")
.withArgs(referral, stakedTokens, receivedShares)

expect(await acre.balanceOf(stakerAddress)).to.be.eq(amountToStake)
nkuba marked this conversation as resolved.
Show resolved Hide resolved
expect(await tbtc.balanceOf(stakerAddress)).to.be.eq(
balanceOfBeforeStake - amountToStake,
)
})

it("should not revert if the referral is zero value", async () => {
const emptyReferral = ethers.encodeBytes32String("")

await expect(
acre
.connect(staker)
.stake(amountToStake, staker.address, emptyReferral),
).to.be.not.reverted
})

it("should revert if a staker wants to stake more tokens than approved", async () => {
await expect(
acre
.connect(staker)
.stake(amountToStake + 1n, staker.address, referral),
).to.be.reverted
})
})

context("when there are two stakers, A and B ", () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
context("when there are two stakers, A and B ", () => {
context("when there are two stakers", () => {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

type Staker = {
signer: HardhatEthersSigner
address: string
nkuba marked this conversation as resolved.
Show resolved Hide resolved
amountToStake: bigint
}
let stakerA: Staker
let stakerB: Staker
let afterStakesSnapshot: SnapshotRestorer
let afterSimulatingYieldSnapshot: SnapshotRestorer

before(async () => {
const [staker1, staker2] = await ethers.getSigners()
const staker1AmountToStake = WeiPerEther * 10000n
const staker2AmountToStake = WeiPerEther * 5000n
// Infinite approval for staking contract.
await tbtc
.connect(staker1)
.approve(await acre.getAddress(), ethers.MaxUint256)
await tbtc
.connect(staker2)
.approve(await acre.getAddress(), ethers.MaxUint256)

// Mint tokens.
await tbtc.connect(staker1).mint(staker1.address, staker1AmountToStake)
await tbtc.connect(staker2).mint(staker2.address, staker2AmountToStake)

stakerA = {
signer: staker1,
address: staker1.address,
amountToStake: staker1AmountToStake,
}
stakerB = {
signer: staker2,
address: staker2.address,
amountToStake: staker2AmountToStake,
}
})

context(
"when the vault is empty and has not yet earned yield from strategies",
() => {
after(async () => {
afterStakesSnapshot = await takeSnapshot()
})

context("when staker A stakes tokens", () => {
it("should stake tokens", async () => {
await expect(
acre
.connect(stakerA.signer)
.stake(stakerA.amountToStake, stakerA.address, referral),
).to.be.not.reverted
})

it("should receive shares equal to a staked amount", async () => {
const shares = await acre.balanceOf(stakerA.address)

expect(shares).to.eq(stakerA.amountToStake)
})
})

context("when staker B stakes tokens", () => {
it("should stake tokens correctly", async () => {
await expect(
acre
.connect(stakerB.signer)
.stake(stakerB.amountToStake, stakerB.address, referral),
).to.be.not.reverted
})

it("should receive shares equal to a staked amount", async () => {
const shares = await acre.balanceOf(stakerB.address)

expect(shares).to.eq(stakerB.amountToStake)
})
})
},
)

context("when the vault has stakes", () => {
before(async () => {
await afterStakesSnapshot.restore()
})

it("the total assets amount should be equal to all staked tokens", async () => {
const totalAssets = await acre.totalAssets()

expect(totalAssets).to.eq(
stakerA.amountToStake + stakerB.amountToStake,
)
})
})

context("when vault earns yield", () => {
let stakerASharesBefore: bigint
let stakerBSharesBefore: bigint
let vaultYield: bigint

before(async () => {
await afterStakesSnapshot.restore()

stakerASharesBefore = await acre.balanceOf(stakerA.address)
stakerBSharesBefore = await acre.balanceOf(stakerB.address)
vaultYield = WeiPerEther * 10000n

// Simulating yield returned from strategies. The vault now contains
// more tokens than deposited which causes the exchange rate to
// change.
await tbtc.mint(await acre.getAddress(), vaultYield)
})

after(async () => {
afterSimulatingYieldSnapshot = await takeSnapshot()
})

it("the vault should hold more assets", async () => {
expect(await acre.totalAssets()).to.be.eq(
stakerA.amountToStake + stakerB.amountToStake + vaultYield,
)
})

it("the staker's shares should be the same", async () => {
expect(await acre.balanceOf(stakerA.address)).to.be.eq(
stakerASharesBefore,
)
expect(await acre.balanceOf(stakerB.address)).to.be.eq(
stakerBSharesBefore,
)
})

it("the staker A should be able to redeem more tokens than before", async () => {
const shares = await acre.balanceOf(stakerA.address)
const availableAssetsToRedeem = await acre.previewRedeem(shares)

expect(availableAssetsToRedeem).to.be.greaterThan(
dimpar marked this conversation as resolved.
Show resolved Hide resolved
nkuba marked this conversation as resolved.
Show resolved Hide resolved
stakerA.amountToStake,
)
})

it("the staker B should be able to redeem more tokens than before", async () => {
const shares = await acre.balanceOf(stakerB.address)
const availableAssetsToRedeem = await acre.previewRedeem(shares)

expect(availableAssetsToRedeem).to.be.greaterThan(
stakerB.amountToStake,
)
})
})

context("when staker A stakes more tokens", () => {
dimpar marked this conversation as resolved.
Show resolved Hide resolved
let sharesBefore: bigint
let availableToRedeemBefore: bigint

before(async () => {
await afterSimulatingYieldSnapshot.restore()

sharesBefore = await acre.balanceOf(stakerA.address)
availableToRedeemBefore = await acre.previewRedeem(sharesBefore)

tbtc.mint(stakerA.address, stakerA.amountToStake)

await acre.stake(stakerA.amountToStake, stakerA.address, referral)
})

it("should receive more shares", async () => {
const shares = await acre.balanceOf(stakerA.address)
const availableToRedeem = await acre.previewRedeem(shares)

expect(shares).to.be.greaterThan(sharesBefore)
expect(availableToRedeem).to.be.greaterThan(availableToRedeemBefore)
})
})
})
})
})
Loading
Loading