Optimize storage of balance checkpoints in T token contract #19
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
cygnusv
commented
Oct 5, 2021
cygnusv
commented
- Uses new implementation for GovernorBravo's Checkpoints borrowed from OpenZeppelin, but with an additional optimization that fits 2 checkpoints per storage slot.
- This produces, on average, a reduction of 20,000 gas per transfer (from 108,000 to 88,000)
- Since we will use checkpoints functionality in the T staking contract and for the covT token, I extracted this functionality to an abstract contract.
- Adds checkpoints to track historical total supply (required for Threshold DAO governance parameters that are defined as percentages of total supply)
* Based on OpenZeppelin's ERC20Votes, that uses an array instead of a mapping to store checkpoints * Instead of storing structs (that always use full slots), use uint128 as the base type. In combination with the array structure, 2 checkpoints fit in 1 storage slot * Maintains the Checkpoint struct as API
* Reduces average transfer cost from 108k gas to 88k. * Removes ~130 LOC from T token contract by using common code from the Checkpoints contract and OpenZeppelin utils
Necessary to test some checkpoint features
pdyraga
reviewed
Oct 6, 2021
| const block = await mineBlock() | ||
| expect(await t.getPastTotalSupply(block - 1)).to.equal(initialBalance) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Maybe we can remove this test and expand context("when minted several times", at line 822 into:
context("when minted just once", () => {
it("should keep track of historic supply", async () => {
const lastBlock = await mineBlock()
expect(await t.getPastTotalSupply(lastBlock - 1)).to.equal(
initialBalance
)
})
})
context("when minted several times", () => {
context("when minted to the same account", () => {
let block1
let block2
let block3
beforeEach(async () => {
await t.connect(thirdParty).delegate(delegatee.address)
await t.connect(deployer).mint(thirdParty.address, to1e18(10))
block1 = await lastBlockNumber()
await t.connect(deployer).mint(thirdParty.address, to1e18(12))
block2 = await lastBlockNumber()
await t.connect(deployer).mint(thirdParty.address, to1e18(15))
block3 = await lastBlockNumber()
await t.connect(deployer).mint(thirdParty.address, to1e18(17))
})
it("should update current votes", async () => {
expect(await t.getCurrentVotes(thirdParty.address)).to.equal(0)
expect(await t.getCurrentVotes(delegatee.address)).to.equal(
to1e18(54)
)
})
it("should keep track of prior votes", async () => {
expect(await t.getPriorVotes(delegatee.address, block1)).to.equal(
to1e18(10)
)
expect(await t.getPriorVotes(delegatee.address, block2)).to.equal(
to1e18(22)
)
expect(await t.getPriorVotes(delegatee.address, block3)).to.equal(
to1e18(37)
)
})
it("should keep track of historic supply", async () => {
expect(await t.getPastTotalSupply(block1 - 1)).to.equal(
initialBalance
)
expect(await t.getPastTotalSupply(block2 - 1)).to.equal(
initialBalance.add(to1e18(10))
)
expect(await t.getPastTotalSupply(block3 - 1)).to.equal(
initialBalance.add(to1e18(22))
)
})
})
context("when minted to different accounts", () => {
let block1
let block2
let block3
beforeEach(async () => {
await t.connect(tokenHolder).delegate(delegatee.address)
await t.connect(thirdParty).delegate(delegatee.address)
await t.connect(deployer).mint(tokenHolder.address, to1e18(10))
block1 = await lastBlockNumber()
await t.connect(deployer).mint(thirdParty.address, to1e18(11))
block2 = await lastBlockNumber()
await t.connect(deployer).mint(tokenHolder.address, to1e18(12))
block3 = await lastBlockNumber()
await t.connect(deployer).mint(thirdParty.address, to1e18(13))
})
it("should update current votes", async () => {
expect(await t.getCurrentVotes(tokenHolder.address)).to.equal(0)
expect(await t.getCurrentVotes(thirdParty.address)).to.equal(0)
expect(await t.getCurrentVotes(delegatee.address)).to.equal(
initialBalance.add(to1e18(46))
)
})
it("should keep track of prior votes", async () => {
expect(await t.getPriorVotes(delegatee.address, block1)).to.equal(
initialBalance.add(to1e18(10))
)
expect(await t.getPriorVotes(delegatee.address, block2)).to.equal(
initialBalance.add(to1e18(21))
)
expect(await t.getPriorVotes(delegatee.address, block3)).to.equal(
initialBalance.add(to1e18(33))
)
})
it("should keep track of historic supply", async () => {
expect(await t.getPastTotalSupply(block1 - 1)).to.equal(
initialBalance
)
expect(await t.getPastTotalSupply(block2 - 1)).to.equal(
initialBalance.add(to1e18(10))
)
expect(await t.getPastTotalSupply(block3 - 1)).to.equal(
initialBalance.add(to1e18(21))
)
})
})
})
There was a problem hiding this comment.
... and then, I think we can get rid of all other historical supply should be tracked test cases for minting. All three basic cases are covered (one mint, several mints to one account, several mints to multiple accounts) and we even have delegations mixed-in and not affecting the result.
There was a problem hiding this comment.
... and I'd do the same for burn: expand context("when burned several times" and have one burn, burn from one account, burn from multiple accounts and remove other getPastTotalSupply checks - we do not really need to distinguish self-delegation or external-delegation, all we care that delegation does not affect the result and we'll have it achieved by these three tests. Plus, we will test more cases specific to getPastTotalSupply, which is burning from multiple accounts and burning just once.
|
I am surprised by
Here: |
`getCurrentVotes` and `getPriorVotes` from T can be substituted by `getVotes` and `getPastVotes` from Checkpoints
Yes, we weren't storing any checkpoints for total supply, which only changes when minting and burning, so this cost increase is expected. |
vzotova
approved these changes
Oct 7, 2021
| uint128[] storage ckpts, | ||
| function(uint256, uint256) view returns (uint256) op, | ||
| uint256 delta | ||
| ) internal returns (uint256 oldWeight, uint256 newWeight) { |
There was a problem hiding this comment.
Can weights also be uint96? And op and amounts in other places? or there are issues with that?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.