-
Notifications
You must be signed in to change notification settings - Fork 22
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
Erc20flash mint #407
base: main
Are you sure you want to change the base?
Erc20flash mint #407
Conversation
✅ Deploy Preview for contracts-stylus canceled.
|
✅ Deploy Preview for contracts-stylus canceled.
|
This is my initial draft |
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.
Hi @Ifechukwudaniel!
Thanks for the contribution!
The PR overall looks good, let's polish it a bit
contracts/src/flashloan/borrower.rs
Outdated
|
||
use stylus_sdk::stylus_proc::sol_interface; | ||
|
||
sol_interface! { |
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.
@bidzyyys @qalisander @ggonzalez94 on the one hand this should be defined inside contracts/src/token/erc20/extensions/flashmint.rs, similar to how IERC721Receiver
is defined inside contracts/src/token/erc721/mod.rs
But on the other hand, Solidity version treats this trait (interface) as a standalone file within contracts/interfaces, similar to how we've done by placing Erc1155Receiver in the same folder as Erc1155
.
What is the convention we want to use here?
EDIT: actually both examples I linked to (IERC721Receiver and Erc1155Receiver) technically use a similar convention, the difference being the latter's module is just placed in a separate file.
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.
Making a whole file is not necessary and is not the way it has been done in the codebase. Instead, what I did was add it to the utility of ERC-20, as it is isolated to ERC-20
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.
To follow the existing convention, place IERC3156FlashBorrower
inside contracts/src/token/erc20/extensions/flashmint.rs (see the EDIT in my previous message).
pub fn _flash_fee_receiver(&self) -> Address { | ||
Address::ZERO | ||
} | ||
} |
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.
Missing unit tests
pub fn _flash_fee_receiver(&self) -> Address { | ||
Address::ZERO | ||
} | ||
} |
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.
Missing integration tests
pub fn _flash_fee_receiver(&self) -> Address { | ||
Address::ZERO | ||
} | ||
} |
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.
Update CHANGELOG
pub fn _flash_fee_receiver(&self) -> Address { | ||
Address::ZERO | ||
} | ||
} |
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.
Update antora docs docs/modules/ROOT/pages/erc20.adoc
Co-authored-by: Nenad <[email protected]>
Co-authored-by: Nenad <[email protected]>
chore: fixed tab
Create erc20-flashloan.adoc
chore: fixes
@0xNeshi what do you think about the new implementation for the extension |
Resolves #355
PR Checklist