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.
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
VIP-14 Morpho PCV Deposit #128
VIP-14 Morpho PCV Deposit #128
Changes from 16 commits
5ba0def
344cbc0
95368fe
85e18d2
fc92e6b
86ea05e
43ac3fb
c9ff1b1
38f7cdb
cf6c21a
c0ddda9
96c3b93
09a11e5
1b11bda
3a927a5
288fb42
6323a8e
f86be20
d72ddea
175b45c
02453fa
64177bb
41b1496
513d872
c231877
72101fa
979ce31
376e7d4
b3a1843
101415e
729fc01
64a50dd
4c0e890
6619e94
21d0bf7
1ce3129
248a693
97b9255
3358ab0
c56434f
649ec46
6de5056
e83c195
6ca9cc5
4999583
d2a78e0
cb8ccd0
1c06df3
79a6981
91a817b
8f6b3d6
397642b
28a501a
a2fcf0a
49e18cd
5a03f63
1a6b664
a71e3b3
bc52fa4
a96025a
2073f25
024edd3
3056222
457613d
42d92da
26be8a2
81cba10
0a90a64
ab9c93e
1fc86aa
72a4567
c8ca5d7
414b443
4d59814
52f3ef6
d1e2632
513e3a9
9c14d09
072b88d
a1e9b2f
8519129
d7c9f6c
ab72cd2
6cd4317
d2c285e
7fa0d40
0101e8d
f867262
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would gate this to a new role, for instance
keccak256(MAPLE_DEPOSIT_ROLE)
, to avoid unintended interactions from the ERC20Allocator / PCVGuardian that are PCV ControllersThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ERC20Allocator is not going to be connected to this deposit, and PCVGuardian cannot call deposit, only withdraw.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any idea why they have this custody locking mechanism ? Seems to me that it would be simpler to just transfer the erc20 tokens, it works but it's a weird pattern?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's likely due to the token lockup period where you can't transfer during lockup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's interestingly similar to AAVE/stkAAVE
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd gate this to a specific
keccak256(MAPLE_WITHDRAW_ROLE)
as starting the cooldown is low severity, this could be granted to the team multisig for instance, and then a full governance timelock is needed to call the actual withdraw because it would be gated to PCV Controller (or go through guardian)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a more mature system state, I would agree that adding additional roles and separating concerns makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd also gate this to
keccak256(MAPLE_WITHDRAW_ROLE)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not like the idea of adding a new role here. It adds complexity and additional cognitive overhead when trying to reason about which addresses are allowed to act on protocol components. Adding roles that are this contract specific creates a multiplicative explosion of new roles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
either remove this from
withdraw()
or emit theHarvest
event here too, or it could break accounting from events