feat: added erc 4626 token standard example (#538) #602

ebadiere · 2023-11-20T19:08:04Z

OZ ERC-4626 TokenVault example.

Related issue(s): #538

Fixes #538

Notes for reviewer:
The test coverage in this PR checks the encoded transaction data coming back in the receipt query response. Local node PR 457 needs to be merged into the local node in order for the contract revert reasons to the test correctly.

Checklist

Documented (Code comments, README, etc.)
Tested (unit, integration, etc.)

Signed-off-by: Logan Nguyen <[email protected]>

…n(); Signed-off-by: Logan Nguyen <[email protected]>

Signed-off-by: Logan Nguyen <[email protected]>

Signed-off-by: ebadiere <[email protected]>

… node. Signed-off-by: ebadiere <[email protected]>

Signed-off-by: ebadiere <[email protected]>

…til a solution for returning the contract revert reason is found. Signed-off-by: ebadiere <[email protected]>

github-actions · 2023-11-20T19:17:18Z

Test Results

  18 files +  4   86 suites +21 9m 56s ⏱️ + 2m 35s
214 tests +  8 205 ✔️ +  7   6 💤 ±0 3 ❌ +1
260 runs +54 246 ✔️ +48 10 💤 +4 4 ❌ +2

For more details on these failures, see this check.

Results for commit 54d0d41. ± Comparison against base commit 8190545.

♻️ This comment has been updated with latest results.

…nReceipt Signed-off-by: ebadiere <[email protected]>

quiet-node

LG overall but still some comments

quiet-node · 2023-11-22T16:43:07Z

contracts/oz/erc-4626/TokenVault.sol

+        return shareHolders[_user];
+    }   
+
+}


Extra line at EOF

quiet-node · 2023-11-22T16:45:46Z

contracts/oz/erc-4626/TokenVault.sol

+        // Calculate the total asset amount as the sum of the share amount plus 10% of the share amount.
+        uint256 assets = _shares + percent;
+        // calling the redeem function from the ERC-4626 library to perform all the necessary functionality
+        redeem(assets, _receiver, msg.sender);


Is this redeem method sending assets to msg.sender? If so I think a checks-effects-interactions approach would be better in this case to avoid reentrant attacks. Just simply swap the line 49 with this line 47 should fix it I believe.

quiet-node · 2023-11-22T16:49:30Z

contracts/solidity/inhetitance/Main.sol

@@ -14,8 +14,4 @@ contract Main is Base {
    function returnSuper() public view virtual returns (string memory) {
        return super.classIdentifier();
    }
-
-    function destroyContract(address recipient) public {


I tried to removed this last time iirc, and Nana told me that we should still keep this until new Cancun EVM release which will remove the support for selfdestruct opcode

Not sure how this got into the review. I may have run a rebase, but I didn't work on that contract.

quiet-node · 2023-11-22T16:50:36Z

test/oz/erc-4626/TokenVault.js

+const { ethers } = require("hardhat");
+const Constants = require('../../constants')
+
+describe("TokenVault Contract", function () {


Please update the unit test flag to @OZTokenValut or @OZerc4626 or something like that

quiet-node · 2023-11-22T16:52:46Z

test/oz/erc-4626/TokenVault.js

+    });
+  });  
+
+});


extra line at EOF

Signed-off-by: ebadiere <[email protected]>

quiet-node

LG. Just 1 nit which I think can be addressed in next PR

quiet-node · 2023-11-28T15:39:16Z

test/oz/erc-4626/TokenVault.js

+const { ethers } = require("hardhat");
+const Constants = require('../../constants')
+
+describe("@OZTokenValut TokenVault Contract", function () {


nit: The OZ flag has a typo

Signed-off-by: ebadiere <[email protected]>

…they may be confusing depending on what node they are running against. Signed-off-by: ebadiere <[email protected]>

Signed-off-by: ebadiere <[email protected]>

quiet-node

LGTM just a couple of nits

quiet-node · 2023-12-11T18:59:03Z

test/oz/erc-4626/TokenVault.js

+const { ethers } = require("hardhat");
+const Constants = require('../../constants')
+
+describe("@OZ TokenVault Contract", function () {


nit: Should be @OZTokenVault

quiet-node · 2023-12-11T19:00:26Z

test/oz/erc-4626/TokenVault.js

+
+  beforeEach(async function () {
+    ERC20Mock = await ethers.getContractFactory("contracts/erc-20/ERC20Mock.sol:ERC20Mock");
+    asset = await ERC20Mock.deploy("MockToken", "MTK", Constants.GAS_LIMIT_1_000_000);


Please make "MockToken" and "MTK" constant variables as I see them being reused around in the contract

quiet-node and others added 11 commits October 27, 2023 12:43

update(docs): upgraded OZ packages to latest version 5.0.0

4acdef3

Signed-off-by: Logan Nguyen <[email protected]>

update: removed ERC20SnapshotMock contract

8c26c56

Signed-off-by: Logan Nguyen <[email protected]>

update: replaced OZ/SafeMath.sol to OZ/Math.sol

ef55ecd

Signed-off-by: Logan Nguyen <[email protected]>

fix: replaced _getImplementation() with ERC1967Utils.getImplementatio…

dc15e01

…n(); Signed-off-by: Logan Nguyen <[email protected]>

fix: passed arguments to the new Ownable base constructor

2de2dcf

Signed-off-by: Logan Nguyen <[email protected]>

fix: fixed warnings

e586193

Signed-off-by: Logan Nguyen <[email protected]>

update: re-compiled all contracts

20f4858

Signed-off-by: Logan Nguyen <[email protected]>

WIP. Adding tests around the OZ ERC-4626 Token vault example.

97966be

Signed-off-by: ebadiere <[email protected]>

WIP. Completed coverage. Needs to be updated to run against the local…

68ecfdb

… node. Signed-off-by: ebadiere <[email protected]>

Merge branch 'main' into 538-ERC-4626-token-standard-example

ec33d2d

Signed-off-by: ebadiere <[email protected]>

Updates to contract revert tests to check encoded transaction data un…

39a9c1f

…til a solution for returning the contract revert reason is found. Signed-off-by: ebadiere <[email protected]>

ebadiere added the enhancement New feature or request label Nov 20, 2023

ebadiere added this to the 0.7.0 milestone Nov 20, 2023

ebadiere self-assigned this Nov 20, 2023

ebadiere requested review from AlfredoG87, AleksandrinaAleksieva, georgi-l95, Ivo-Yankov, natanasow, yiliev0, a team and Nana-EC as code owners November 20, 2023 19:08

Updated contract revert issue to the relay issue. #1916 getTransactio…

c010eef

…nReceipt Signed-off-by: ebadiere <[email protected]>

quiet-node requested changes Nov 22, 2023

View reviewed changes

Applied feedback

4533527

Signed-off-by: ebadiere <[email protected]>

quiet-node previously approved these changes Nov 28, 2023

View reviewed changes

Set the revert reasons on.

c5c1493

Signed-off-by: ebadiere <[email protected]>

ebadiere dismissed quiet-node’s stale review via c5c1493 November 30, 2023 15:05

ebadiere marked this pull request as draft November 30, 2023 15:06

Refactored to evaulate the results of eth_estimateGase.

3b3b8a7

Signed-off-by: ebadiere <[email protected]>

ebadiere marked this pull request as ready for review December 6, 2023 18:10

ebadiere added 2 commits December 8, 2023 15:23

Removed two tests that covered zero deposit and withdraw amounts, as …

3d9f724

…they may be confusing depending on what node they are running against. Signed-off-by: ebadiere <[email protected]>

Cleaned up label.

54d0d41

Signed-off-by: ebadiere <[email protected]>

ebadiere changed the title ~~538 erc 4626 token standard example~~ feat: 538 erc 4626 token standard example Dec 11, 2023

quiet-node approved these changes Dec 11, 2023

View reviewed changes

quiet-node changed the title ~~feat: 538 erc 4626 token standard example~~ feat: added erc 4626 token standard example (#538) Dec 12, 2023

quiet-node merged commit 81df819 into main Dec 12, 2023
22 of 24 checks passed

quiet-node deleted the 538-ERC-4626-token-standard-example branch December 12, 2023 22:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: added erc 4626 token standard example (#538) #602

feat: added erc 4626 token standard example (#538) #602

ebadiere commented Nov 20, 2023 •

edited

Loading

github-actions bot commented Nov 20, 2023 •

edited

Loading

quiet-node left a comment

quiet-node Nov 22, 2023

ebadiere Nov 22, 2023

quiet-node Nov 22, 2023 •

edited

Loading

ebadiere Nov 22, 2023

quiet-node Nov 22, 2023

ebadiere Nov 22, 2023

quiet-node Nov 22, 2023

ebadiere Nov 22, 2023

quiet-node Nov 22, 2023

ebadiere Dec 11, 2023

quiet-node left a comment

quiet-node Nov 28, 2023

ebadiere Dec 8, 2023

quiet-node left a comment

quiet-node Dec 11, 2023

quiet-node Dec 11, 2023

+                  });
+                });
+              });

feat: added erc 4626 token standard example (#538) #602

feat: added erc 4626 token standard example (#538) #602

Conversation

ebadiere commented Nov 20, 2023 • edited Loading

github-actions bot commented Nov 20, 2023 • edited Loading

Test Results

quiet-node left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quiet-node Nov 22, 2023 • 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

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

quiet-node left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quiet-node left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebadiere commented Nov 20, 2023 •

edited

Loading

github-actions bot commented Nov 20, 2023 •

edited

Loading

quiet-node Nov 22, 2023 •

edited

Loading