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
first version of fast-withdrawal feature is ready #24
base: pa
Are you sure you want to change the base?
first version of fast-withdrawal feature is ready #24
Changes from 1 commit
31b4191
7d105a4
9766972
48c3950
8904d1a
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.
in most other contracts we handle the treasury as immutable as there's no good reason for it to ever change
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 added a setter here because the possibility of distributing these rewards to users as additional APY was previously discussed.
So I decided to leave some flexibility here.
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.
don't understand that comment, can you elaborate?
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.
The comment refers to the same division of roles as stated below.
It is not clear why the owner is able to move my funds.
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 aave contracts owner is always governance short exec, imo doesn't make sense to change the naming now
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 think it’s a good pattern to have 2 different roles that will be responsible for helping/calling their methods.
Being an auditor or any third party not familiar with the specifics of implementation in AAVE, I will have questions about why the owner (regardless of whether it is a DAO or some kind of contract or address) can withdraw my money specifically to any address.
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 personally agree, but would say it's to opinionated - better to follow the standard.
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 absolutely unreadable to have such scattered functions. It’s just difficult for me to understand what entry points the contract has and what methods are available inside.
I don’t consider this opinioned, at least because out of 40 projects I have never seen anyone write this way. This is at least some informations about a bad style guide and unreadable code.