Flex voting Atoken using scaled balance caching #21
Conversation
Why this works: #8 (comment)
davidlaprade
force-pushed
the
atoken-caching-approach-fixes-#8
branch
from
1f8122d to
aca43e4
Compare
davidlaprade
force-pushed
the
atoken-caching-approach-fixes-#8
branch
from
4cd8077 to
7efdd7b
Compare
src/ATokenCheckpointed.sol
Outdated
| /// Note: this has been modified from Aave v3's AToken to call our custom | ||
| /// mintScaledWithCheckpoint function. | ||
| /// | ||
| /// @inheritdoc IAToken | ||
| function mintToTreasury(uint256 amount, uint256 index) external override onlyPool { | ||
| if (amount == 0) { | ||
| return; | ||
| } | ||
| _mintScaledWithCheckpoint(address(POOL), _treasury, amount, index); | ||
| } |
There was a problem hiding this comment.
As I'm reviewing this, I actually think we should not be checkpointing this function. It's called by the pool when someone calls mintToTreasury.
But mintToTreasury does not involve depositing any underlying tokens into Aave (even though it results in minting aTokens). So there should be no need to checkpoint anything. Adding checkpoints here would actually mess up our internal accounting.
Furthermore, the treasury (which is the address that we would be checkpointing for) is not an entity that can vote
There was a problem hiding this comment.
I don't get what this mintToTreasury function is for? It's basically just AToken inflation by minting ATokens with no underlying assets involved? But it seems anyone can call Pool.mintToTreasury, so I must be missing something
There was a problem hiding this comment.
Don't fully understand here either, but this code seems to indicate the treasury is earning some income, so perhaps minting to the treasury is just giving the treasury aTokens for the income it has already earned in the pool (fees?). If that's the case, checkpointing would be warranted, I believe.
There was a problem hiding this comment.
minting to the treasury is just giving the treasury aTokens for the income it has already earned in the pool (fees?). If that's the case, checkpointing would be warranted, I believe
I think that's right: it's minting aTokens to represent the treasury income, which are fees collected at various points, e.g. here and here.
Now that I think more about it, it probably does make sense to include this. We're snapshotting aToken balances now, not deposits. And, while the treasury won't have made any deposits, it will have an aToken balance because of this. If at some point Aave decides it wants to participate in governance with the aTokens it has earned as fees, it should be able to. Thanks for correcting me 👍
I should probably add a test for this functionality though
There was a problem hiding this comment.
Tested in ba4a946
There was a problem hiding this comment.
@davidlaprade for posterity can you leave a comment here summarizing what mintToTreasury is for, when/who calls it, and whether we need to care about it? The commit linked above was rebased away and the most recent comments in this thread are "I think" style comments, e.g. from reading it I'm not certain if that's correct or outdated
There was a problem hiding this comment.
Sure!
mintToTreasury captures/realizes protocol fees by issuing aTokens to the Aave treasury address.
The function is public, so it can be called by anyone. Ernesto from Aave told us in Telegram that it's called by "somebody in the community I think".
We do need to care about it, for the reason @apbendi stated. ATokens are being issued and the underlying asset could potentially be withdrawn (and used for voting). For consistency, therefore, we should checkpoint when it happens and accord the appropriate voting weight to the Treasury -- even if we think it will never express a voting preference.
We do support this function now, and are checkpointing when it is called, as seen by the test added in ba4a946
src/ATokenCheckpointed.sol
Outdated
| // Begin modifications. | ||
| _checkpointRawBalanceOf(_msgSender()); | ||
| _checkpointRawBalanceOf(recipient); | ||
| // End modifications. |
There was a problem hiding this comment.
^^ the internal _transfer instead
agreed
it seems like there are quite a few internal _transfers! We should be sure to override the right one (aka should checkpoint on both AToken:_transfer fns, the one with a validate bool and the one without)
There was a problem hiding this comment.
Looks great @davidlaprade, left some additional comments. Mostly minor!
src/ATokenCheckpointed.sol
Outdated
| /// Note: this has been modified from Aave v3's AToken to call our custom | ||
| /// mintScaledWithCheckpoint function. | ||
| /// | ||
| /// @inheritdoc IAToken | ||
| function mintToTreasury(uint256 amount, uint256 index) external override onlyPool { | ||
| if (amount == 0) { | ||
| return; | ||
| } | ||
| _mintScaledWithCheckpoint(address(POOL), _treasury, amount, index); | ||
| } |
There was a problem hiding this comment.
Don't fully understand here either, but this code seems to indicate the treasury is earning some income, so perhaps minting to the treasury is just giving the treasury aTokens for the income it has already earned in the pool (fees?). If that's the case, checkpointing would be warranted, I believe.
This is so much simpler/cleaner
davidlaprade
force-pushed
the
atoken-caching-approach-fixes-#8
branch
from
edb5661 to
d092fb7
Compare
davidlaprade
force-pushed
the
atoken-caching-approach-fixes-#8
branch
from
b7f9625 to
f92e600
Compare
davidlaprade
force-pushed
the
atoken-caching-approach-fixes-#8
branch
from
ba4a946 to
f353412
Compare
Fixes #8
More details about this approach and why I think it works can be found here #8 (comment)