feat: add ERC20FlashMint extension #407

Ifechukwudaniel · 2024-11-14T04:27:09Z

NOTE: It is impossible to write any unit tests until it is possible to mock contract::address() in tests (see OpenZeppelin/stylus-test-helpers#5).

Resolves #355

PR Checklist

Tests
Documentation
Changelog

netlify · 2024-11-14T04:27:26Z

✅ Deploy Preview for contracts-stylus canceled.

Name	Link
🔨 Latest commit	`a7c0160`
🔍 Latest deploy log	https://app.netlify.com/sites/contracts-stylus/deploys/67357c1f782e3e00081ecd3f

netlify · 2024-11-14T04:27:45Z

✅ Deploy Preview for contracts-stylus canceled.

Name	Link
🔨 Latest commit	`1ba51fb`
🔍 Latest deploy log	https://app.netlify.com/sites/contracts-stylus/deploys/6769107b888750000807398f

Ifechukwudaniel · 2024-11-14T04:27:46Z

This is my initial draft

0xNeshi

Hi @Ifechukwudaniel!
Thanks for the contribution!

The PR overall looks good, let's polish it a bit

contracts/src/flashloan/borrower.rs

contracts/src/token/erc20/extensions/flashmint.rs

0xNeshi · 2024-11-14T08:29:50Z

contracts/src/token/erc20/extensions/flashmint.rs

+    pub fn _flash_fee_receiver(&self) -> Address {
+        Address::ZERO
+    }
+}


Missing unit tests

0xNeshi · 2024-11-14T08:30:11Z

contracts/src/token/erc20/extensions/flashmint.rs

+    pub fn _flash_fee_receiver(&self) -> Address {
+        Address::ZERO
+    }
+}


Missing integration tests

contracts/src/token/erc20/extensions/flashmint.rs

Co-authored-by: Nenad <[email protected]>

chore: fixed tab

Create erc20-flashloan.adoc

chore: fixes

Ifechukwudaniel · 2024-11-18T03:21:35Z

@0xNeshi what do you think about the new implementation for the extension

0xNeshi · 2024-12-20T08:16:02Z

contracts/src/token/erc20/extensions/flash_mint.rs

+    fn flash_fee(
+        &self,
+        token: Address,
+        amount: U256,


It is not clear which terminology to use here:

ERC20 uses value (see transfer def)

ERC3156 uses amount (see EIP)

The Solidity team had the same problem. They opted for keeping things consistent with ERC20. Our implementation does not; instead it is consistent with the EIP.

Wdyt?

Isn't it in solidity value is used?

Yea, I mentioned that above:

They opted for keeping things consistent with ERC20

Should we align with the Solidity team or with official EIP, wdyt?

qalisander

Good job!
I have a few comments and I'm also concerned about solidity files, that we copied from solidity repo.
Can we use a url instead of copying?

qalisander · 2024-12-20T09:48:29Z

contracts/src/token/erc20/extensions/flash_mint.rs

+    fn flash_fee(
+        &self,
+        token: Address,
+        amount: U256,


Isn't it in solidity value is used?

qalisander · 2024-12-20T10:10:14Z

examples/erc20-flash-mint/src/ERC3156FlashBorrowerMock.sol

+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.24;
+
+import "../../solidity-contracts/contracts/interfaces/IERC3156FlashBorrower.sol";


Can't we import them from by url like here

solc doesn't work with urls. The reason we can do this in other Solidity files is that those files are, to be frank, useless - they're never used anywhere, so no builds are crashed.

I'm not even sure why we have them in the repo tbh 🤔

qalisander · 2024-12-20T10:17:38Z

examples/solidity-contracts/contracts/token/ERC20/IERC20.sol

+/**
+ * @dev Interface of the ERC-20 standard as defined in the ERC.
+ */
+interface IERC20 {


I hope we should avoid to copy sol files from solidity library and just use url references

The problem is having to deploy ERC3156FlashBorrowerMock contract to be able to run in bench for flash_loan (see bench); and to be able to implement this contract locally, it seems necessary to have all the dependencies locally as well...

Do you if there's a way to import existing examples/erc20-flash-mint/tests/mock/borrower.rs implementation into benches and reuse it? 🤔 This would make the duplicate here redundant.

qalisander · 2024-12-20T10:28:50Z

contracts/src/token/erc20/extensions/flash_mint.rs

+        address!("dce82b5f92c98f27f116f70491a487effdb6a2aa");
+
+    #[motsu::test]
+    #[ignore]


I see that we can check these test cases with approach similar to Erc721 Metadata. Declare a contract that contains Erc20 and Erc20FlashMint inside. Similar to what you have in FlashMintExample.

Then test cases should work

Otherwise we cannot use two contract's storages in motsu test case

Since FlashMint relies upon contract::address() make sense to wait a bit, when motsu can support it

0xNeshi · 2024-12-23T07:11:32Z

.github/workflows/test.yml

+        # The `erc20-flash-mint-example` package enables the `reentrant` feature
+        # on `openzeppelin-stylus`, which combined with `--all-targets` seems to
+        # cause a linker issue "error: undefined symbol: storage_flush_cache".
+        #
+        # This is a known problem with workspace feature unification:
+        # https://github.com/rust-lang/cargo/issues/4463
+        #
+        # To deal with this, we simply ignore `erc20-flash-mint-example` from the compilation.
+        run: cargo nextest run --locked --features std --all-targets --workspace --exclude erc20-flash-mint-example


I think we could omit the whole of examples/* directory for this step, as we're only interested in unit tests, it would speed up the step's execution

Ifechukwudaniel added 3 commits November 13, 2024 21:31

chore: Implemented ERC3156FlashLender and ERC3156FlashBorrower

ab91bbe

chore: removed lender

7862012

chore: flashmint

a7c0160

Ifechukwudaniel requested review from bidzyyys, qalisander and 0xNeshi as code owners November 14, 2024 04:27

Merge branch 'main' into erc20flashMint

70d67a1

0xNeshi reviewed Nov 14, 2024

View reviewed changes

Ifechukwudaniel and others added 18 commits November 14, 2024 12:08

chore : cargo format

47053bc

Update contracts/src/token/erc20/extensions/flashmint.rs

6ea75eb

Co-authored-by: Nenad <[email protected]>

Update contracts/src/token/erc20/extensions/flashmint.rs

53ee21a

Co-authored-by: Nenad <[email protected]>

chore: added docs for IERC3156FlashBorrower

3bd25cf

chore: fixed tab

chore: docs

b9cce5d

Merge branch 'main' into erc20flashMint

03f6f38

Create erc20-flashloan.adoc

6aa5b6d

Merge pull request #1 from Ifechukwudaniel/patch-1

ebee8d5

Create erc20-flashloan.adoc

chore: removed flashlaon and smaller changes

55672f6

chore: cargo format

626ae03

chore: abi and constructor

f8cdcf2

chore: flashloan example setup

85e62b1

chore: changed erc20FlashMint to erc20FlashMint

5feeb90

chore: erc20flashmint example

47aad90

chore: mock, abi and initial e2e test

99a8089

chore: created Erc3156FlashBorrowerMock

1b20ea6

chore: minor changes

4d08485

chore: cargo fmt fix

c3271bf

chore: fixes

0xNeshi added 5 commits December 20, 2024 08:09

chore: Merge remote-tracking branch 'upstream/main' into erc20flashMint

639174c

test: move flash_loan_with_fee_and_fee_receiver up

775bd83

ref: _burn allowance value instead of amount+fee

ad83357

test: add more flash_loan tests

3eda8ac

ref(test): rename _enableReturn->_validReturn

bf8e0c4

0xNeshi marked this pull request as draft December 20, 2024 08:05

0xNeshi reviewed Dec 20, 2024

View reviewed changes

0xNeshi added 3 commits December 20, 2024 09:39

test: add cases for insufficient tokens to burn/transfer fees

1f37b64

test: Add case flash_loan_reverts_when_allowance_overflows

4538ebc

test: add happy flow cases with present/missing fee & fee receiver

987d3a3

0xNeshi marked this pull request as ready for review December 20, 2024 08:52

test: remove unused Erc1155 import in uri_storage

f4d41e3

qalisander reviewed Dec 20, 2024

View reviewed changes

ref: remove redundant stylus-sdk version param in toml

d0b5343

0xNeshi force-pushed the erc20flashMint branch from deb20ec to d0b5343 Compare December 20, 2024 10:42

0xNeshi added 9 commits December 20, 2024 13:05

docs: update e2e alloy links

d7cca49

ci: fix linker issue with reentrancy enabled

c58b3a5

chore: update CHANGELOG

86b98ee

ci: exclude examples from code coverage

ec467c2

ci: fix coverage member filtering

c232c48

ci: remove nextest which targets erc20-flash-mint-example

1eab9a8

docs: add link to issue related to commented bench

41ecfae

ref: add missing '-example' to ownable-two-step package name

32528cb

ci: fix filtered_members

165ba8b

0xNeshi requested review from qalisander and 0xNeshi December 20, 2024 13:42

0xNeshi added 2 commits December 20, 2024 14:47

ci: inline members

546d35a

ci: add comment about filtering examples from coverage

d0f6a6e

0xNeshi reviewed Dec 23, 2024

View reviewed changes

ci: remove 'id' & add explanatory comments to filter out examples step

1ba51fb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add ERC20FlashMint extension #407

feat: add ERC20FlashMint extension #407

Ifechukwudaniel commented Nov 14, 2024 •

edited by 0xNeshi

Loading

netlify bot commented Nov 14, 2024 •

edited

Loading

netlify bot commented Nov 14, 2024 •

edited

Loading

Ifechukwudaniel commented Nov 14, 2024

0xNeshi left a comment

0xNeshi Nov 14, 2024

0xNeshi Nov 14, 2024

Ifechukwudaniel commented Nov 18, 2024

0xNeshi Dec 20, 2024 •

edited

Loading

qalisander Dec 20, 2024

0xNeshi Dec 20, 2024 •

edited

Loading

qalisander left a comment •

edited

Loading

qalisander Dec 20, 2024

qalisander Dec 20, 2024

0xNeshi Dec 20, 2024 •

edited

Loading

qalisander Dec 20, 2024

0xNeshi Dec 20, 2024

qalisander Dec 20, 2024

qalisander Dec 20, 2024 •

edited

Loading

qalisander Dec 20, 2024

0xNeshi Dec 23, 2024 •

edited

Loading

feat: add ERC20FlashMint extension #407

Are you sure you want to change the base?

feat: add ERC20FlashMint extension #407

Conversation

Ifechukwudaniel commented Nov 14, 2024 • edited by 0xNeshi Loading

PR Checklist

netlify bot commented Nov 14, 2024 • edited Loading

✅ Deploy Preview for contracts-stylus canceled.

netlify bot commented Nov 14, 2024 • edited Loading

✅ Deploy Preview for contracts-stylus canceled.

Ifechukwudaniel commented Nov 14, 2024

0xNeshi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ifechukwudaniel commented Nov 18, 2024

0xNeshi Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xNeshi Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

qalisander left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xNeshi Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qalisander Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xNeshi Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

Ifechukwudaniel commented Nov 14, 2024 •

edited by 0xNeshi

Loading

netlify bot commented Nov 14, 2024 •

edited

Loading

netlify bot commented Nov 14, 2024 •

edited

Loading

0xNeshi Dec 20, 2024 •

edited

Loading

0xNeshi Dec 20, 2024 •

edited

Loading

qalisander left a comment •

edited

Loading

0xNeshi Dec 20, 2024 •

edited

Loading

qalisander Dec 20, 2024 •

edited

Loading

0xNeshi Dec 23, 2024 •

edited

Loading