Add token wrapper #526
Add token wrapper #526
Conversation
|
WalkthroughThis update introduces a token wrapper contract Changes
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| interface IHATToken is IVotes, IERC20Permit { | ||
| interface IHATToken is IVotes { |
There was a problem hiding this comment.
The IHATToken interface no longer extends the IERC20Permit interface. This change could potentially break any contracts or off-chain services that rely on the permit functionality of the IHATToken. If this is intentional and you've considered the implications, then it's fine. However, if the permit functionality is still needed, you should reconsider this change.
- interface IHATToken is IVotes {
+ interface IHATToken is IVotes, IERC20Permit {
| // SPDX-License-Identifier: MIT | ||
| // Disclaimer https://github.com/hats-finance/hats-contracts/blob/main/DISCLAIMER.md | ||
|
|
||
| pragma solidity 0.8.16; | ||
|
|
||
| import "@openzeppelin/contracts/token/ERC20/extensions/ERC4626.sol"; | ||
|
|
||
| contract HATTokenWrapper is ERC4626 { | ||
|
|
||
| error AmountOfSharesMustBeMoreThanMinimalAmount(); | ||
|
|
||
| uint256 public constant MINIMAL_AMOUNT_OF_SHARES = 1e3; // to reduce rounding errors, the number of shares is either 0, or > than this number | ||
|
|
||
| constructor (IERC20 _asset, string memory _name, string memory _symbol) ERC4626(_asset) ERC20(_name, _symbol) {} | ||
|
|
||
| function _afterTokenTransfer(address, address, uint256) internal virtual override { | ||
| if (totalSupply() > 0 && totalSupply() < MINIMAL_AMOUNT_OF_SHARES) { | ||
| revert AmountOfSharesMustBeMoreThanMinimalAmount(); | ||
| } | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
The HATTokenWrapper contract seems to be well implemented. It correctly extends the ERC4626 contract from OpenZeppelin and overrides the _afterTokenTransfer function to enforce a minimum amount of shares. The use of custom error types for revert reasons is also a good practice as it can make debugging easier. However, there's a minor issue with the constructor. It should call the parent constructors in the correct order.
- constructor (IERC20 _asset, string memory _name, string memory _symbol) ERC4626(_asset) ERC20(_name, _symbol) {}
+ constructor (IERC20 _asset, string memory _name, string memory _symbol) ERC20(_name, _symbol) ERC4626(_asset) {}
| @@ -479,7 +479,7 @@ function committeeCheckedIn() external view returns (bool) | |||
| ### convertToAssets | |||
|
|
|||
| ```solidity | |||
| function convertToAssets(uint256 shares) external view returns (uint256 assets) | |||
| function convertToAssets(uint256 shares) external view returns (uint256) | |||
There was a problem hiding this comment.
The function signature in the documentation has been changed and it no longer includes the variable name for the return type. This could potentially make the documentation less clear.
- function convertToAssets(uint256 shares) external view returns (uint256)
+ function convertToAssets(uint256 shares) external view returns (uint256 assets)
| @@ -496,12 +496,12 @@ function convertToAssets(uint256 shares) external view returns (uint256 assets) | |||
|
|
|||
| | Name | Type | Description | | |||
| |---|---|---| | |||
| | assets | uint256 | undefined | | |||
| | _0 | uint256 | undefined | | |||
There was a problem hiding this comment.
The parameter description in the documentation has been changed from assets to _0, which is less descriptive and could lead to confusion.
- | _0 | uint256 | undefined |
+ | assets | uint256 | undefined |
|
|
||
| ### convertToShares | ||
|
|
||
| ```solidity | ||
| function convertToShares(uint256 assets) external view returns (uint256 shares) | ||
| function convertToShares(uint256 assets) external view returns (uint256) |
There was a problem hiding this comment.
The function signature in the documentation has been changed and it no longer includes the variable name for the return type. This could potentially make the documentation less clear.
- function convertToShares(uint256 assets) external view returns (uint256)
+ function convertToShares(uint256 assets) external view returns (uint256 shares)
| @@ -518,7 +518,7 @@ function convertToShares(uint256 assets) external view returns (uint256 shares) | |||
|
|
|||
| | Name | Type | Description | | |||
| |---|---|---| | |||
| | shares | uint256 | undefined | | |||
| | _0 | uint256 | undefined | | |||
There was a problem hiding this comment.
The parameter description in the documentation has been changed from shares to _0, which is less descriptive and could lead to confusion.
- | _0 | uint256 | undefined |
+ | shares | uint256 | undefined |
|
|
||
| | Name | Type | Description | | ||
| |---|---|---| | ||
| | _0 | string | undefined | |
There was a problem hiding this comment.
The description for the return value of CLOCK_MODE function is marked as undefined. It would be helpful to provide a meaningful description for better understanding of the function's purpose and its return value.
- | _0 | string | undefined |
+ | _0 | string | Description of the clock mode |
|
|
||
| ### isOperationReady | ||
|
|
||
| ```solidity | ||
| function isOperationReady(bytes32 id) external view returns (bool ready) | ||
| function isOperationReady(bytes32 id) external view returns (bool) |
There was a problem hiding this comment.
The return type of the isOperationReady function has been changed from (bool ready) to (bool). This change removes the explicit variable name from the return type, which might make it less clear for users of this function what the returned value represents. Consider adding a comment above the function definition to explain what the returned bool represents.
- function isOperationReady(bytes32 id) external view returns (bool)
+ // Returns true if the operation is ready, false otherwise
+ function isOperationReady(bytes32 id) external view returns (bool ready)
| @@ -547,7 +547,7 @@ function schedule(address target, uint256 value, bytes data, bytes32 predecessor | |||
|
|
|||
|
|
|||
|
|
|||
| *Schedule an operation containing a single transaction. Emits a {CallScheduled} event. Requirements: - the caller must have the 'proposer' role.* | |||
| *Schedule an operation containing a single transaction. Emits {CallSalt} if salt is nonzero, and {CallScheduled}. Requirements: - the caller must have the 'proposer' role.* | |||
There was a problem hiding this comment.
The documentation for the scheduling functions now mentions that a CallSalt event will be emitted if the salt is nonzero. This is a useful addition as it provides more information about the behavior of these functions.
| event CallSalt(bytes32 indexed id, bytes32 salt) | ||
| ``` | ||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
| #### Parameters | ||
|
|
||
| | Name | Type | Description | | ||
| |---|---|---| | ||
| | id `indexed` | bytes32 | undefined | | ||
| | salt | bytes32 | undefined | | ||
|
|
There was a problem hiding this comment.
A new event CallSalt has been added. This event is emitted when an operation with a non-zero salt is scheduled. This is a good addition as it allows tracking of operations with specific salts.
+ // Emitted when an operation with a non-zero salt is scheduled
+ event CallSalt(bytes32 indexed id, bytes32 salt)
| @@ -154,7 +154,7 @@ function executeBatch(address[] targets, uint256[] values, bytes[] payloads, byt | |||
| ### getMinDelay | |||
|
|
|||
| ```solidity | |||
| function getMinDelay() external view returns (uint256 duration) | |||
| function getMinDelay() external view returns (uint256) | |||
There was a problem hiding this comment.
The function signature has been changed to remove the explicit variable name from the return type. This change is not an issue as long as it's consistent across the codebase and the documentation is updated accordingly.
- function getMinDelay() external view returns (uint256 duration)
+ function getMinDelay() external view returns (uint256)
| function getTimestamp(bytes32 id) external view returns (uint256) | ||
| ``` |
There was a problem hiding this comment.
The function signature has been changed to remove the explicit variable name from the return type. This change is not an issue as long as it's consistent across the codebase and the documentation is updated accordingly.
- function getTimestamp(bytes32 id) external view returns (uint256 timestamp)
+ function getTimestamp(bytes32 id) external view returns (uint256)
| @@ -255,7 +255,7 @@ function hasRole(bytes32 role, address account) external view returns (bool) | |||
| ### hashOperation | |||
|
|
|||
| ```solidity | |||
| function hashOperation(address target, uint256 value, bytes data, bytes32 predecessor, bytes32 salt) external pure returns (bytes32 hash) | |||
| function hashOperation(address target, uint256 value, bytes data, bytes32 predecessor, bytes32 salt) external pure returns (bytes32) | |||
There was a problem hiding this comment.
The function signature has been changed to remove the explicit variable name from the return type. This change is not an issue as long as it's consistent across the codebase and the documentation is updated accordingly.
- function hashOperation(address target, uint256 value, bytes data, bytes32 predecessor, bytes32 salt) external pure returns (bytes32 hash)
+ function hashOperation(address target, uint256 value, bytes data, bytes32 predecessor, bytes32 salt) external pure returns (bytes32)
|
|
||
| ### hashOperationBatch | ||
|
|
||
| ```solidity | ||
| function hashOperationBatch(address[] targets, uint256[] values, bytes[] payloads, bytes32 predecessor, bytes32 salt) external pure returns (bytes32 hash) | ||
| function hashOperationBatch(address[] targets, uint256[] values, bytes[] payloads, bytes32 predecessor, bytes32 salt) external pure returns (bytes32) |
There was a problem hiding this comment.
The function signature has been changed to remove the explicit variable name from the return type. This change is not an issue as long as it's consistent across the codebase and the documentation is updated accordingly.
- function hashOperationBatch(address[] targets, uint256[] values, bytes[] payloads, bytes32 predecessor, bytes32 salt) external pure returns (bytes32 hash)
+ function hashOperationBatch(address[] targets, uint256[] values, bytes[] payloads, bytes32 predecessor, bytes32 salt) external pure returns (bytes32)
|
|
||
| ### isOperation | ||
|
|
||
| ```solidity | ||
| function isOperation(bytes32 id) external view returns (bool registered) | ||
| function isOperation(bytes32 id) external view returns (bool) |
There was a problem hiding this comment.
The function signature has been changed to remove the explicit variable name from the return type. This change is not an issue as long as it's consistent across the codebase and the documentation is updated accordingly.
- function isOperation(bytes32 id) external view returns (bool registered)
+ function isOperation(bytes32 id) external view returns (bool)
|
|
||
| ### isOperationDone | ||
|
|
||
| ```solidity | ||
| function isOperationDone(bytes32 id) external view returns (bool done) | ||
| function isOperationDone(bytes32 id) external view returns (bool) |
There was a problem hiding this comment.
The function signature has been changed to remove the explicit variable name from the return type. This change is not an issue as long as it's consistent across the codebase and the documentation is updated accordingly.
- function isOperationDone(bytes32 id) external view returns (bool done)
+ function isOperationDone(bytes32 id) external view returns (bool)
|
|
||
| ### isOperationPending | ||
|
|
||
| ```solidity | ||
| function isOperationPending(bytes32 id) external view returns (bool pending) | ||
| function isOperationPending(bytes32 id) external view returns (bool) |
There was a problem hiding this comment.
The function signature has been changed to remove the explicit variable name from the return type. This change is not an issue as long as it's consistent across the codebase and the documentation is updated accordingly.
- function isOperationPending(bytes32 id) external view returns (bool pending)
+ function isOperationPending(bytes32 id) external view returns (bool)
|
|
||
| ### isOperationReady | ||
|
|
||
| ```solidity | ||
| function isOperationReady(bytes32 id) external view returns (bool ready) | ||
| function isOperationReady(bytes32 id) external view returns (bool) |
There was a problem hiding this comment.
The function signature has been changed to remove the explicit variable name from the return type. This change is not an issue as long as it's consistent across the codebase and the documentation is updated accordingly.
- function isOperationReady(bytes32 id) external view returns (bool ready)
+ function isOperationReady(bytes32 id) external view returns (bool)
| @@ -511,7 +511,7 @@ function schedule(address target, uint256 value, bytes data, bytes32 predecessor | |||
|
|
|||
|
|
|||
|
|
|||
| *Schedule an operation containing a single transaction. Emits a {CallScheduled} event. Requirements: - the caller must have the 'proposer' role.* | |||
| *Schedule an operation containing a single transaction. Emits {CallSalt} if salt is nonzero, and {CallScheduled}. Requirements: - the caller must have the 'proposer' role.* | |||
There was a problem hiding this comment.
The comment has been updated to include information about a new event CallSalt that is emitted when the salt is nonzero. This is a good addition as it provides more context about the function's behavior.
- *Schedule an operation containing a single transaction. Emits a {CallScheduled} event. Requirements: - the caller must have the 'proposer' role.*
+ *Schedule an operation containing a single transaction. Emits {CallSalt} if salt is nonzero, and {CallScheduled}. Requirements: - the caller must have the 'proposer' role.*
| @@ -532,7 +532,7 @@ function scheduleBatch(address[] targets, uint256[] values, bytes[] payloads, by | |||
|
|
|||
|
|
|||
|
|
|||
| *Schedule an operation containing a batch of transactions. Emits one {CallScheduled} event per transaction in the batch. Requirements: - the caller must have the 'proposer' role.* | |||
| *Schedule an operation containing a batch of transactions. Emits {CallSalt} if salt is nonzero, and one {CallScheduled} event per transaction in the batch. Requirements: - the caller must have the 'proposer' role.* | |||
There was a problem hiding this comment.
The comment has been updated to include information about a new event CallSalt that is emitted when the salt is nonzero. This is a good addition as it provides more context about the function's behavior.
- *Schedule an operation containing a batch of transactions. Emits one {CallScheduled} event per transaction in the batch. Requirements: - the caller must have the 'proposer' role.*
+ *Schedule an operation containing a batch of transactions. Emits {CallSalt} if salt is nonzero, and one {CallScheduled} event per transaction in the batch. Requirements: - the caller must have the 'proposer' role.*
| event CallSalt(bytes32 indexed id, bytes32 salt) | ||
| ``` |
There was a problem hiding this comment.
A new event CallSalt has been added. This is a good addition as it provides more information about the state changes in the contract.
+ event CallSalt(bytes32 indexed id, bytes32 salt)
|
|
||
|
|
||
|
|
||
| *Emitted when new proposal is scheduled with non-zero salt.* |
There was a problem hiding this comment.
The comment has been added to describe the new event CallSalt. This is a good addition as it provides more context about the event.
+ *Emitted when new proposal is scheduled with non-zero salt.*
| ### CLOCK_MODE | ||
|
|
||
| ```solidity | ||
| function CLOCK_MODE() external view returns (string) | ||
| ``` | ||
|
|
||
|
|
||
|
|
||
| *Description of the clock* | ||
|
|
||
|
|
||
| #### Returns | ||
|
|
||
| | Name | Type | Description | | ||
| |---|---|---| | ||
| | _0 | string | undefined | | ||
|
|
There was a problem hiding this comment.
The CLOCK_MODE function is missing a description for its return value. It's important to provide clear documentation for all public and external functions.
| _0 | string | undefined |
+26: | _0 | string | Description of the clock mode |
| ### clock | ||
|
|
||
| ```solidity | ||
| function clock() external view returns (uint48) | ||
| ``` | ||
|
|
||
|
|
||
|
|
||
| *Clock used for flagging checkpoints. Can be overridden to implement timestamp based checkpoints (and voting).* | ||
|
|
||
|
|
||
| #### Returns | ||
|
|
||
| | Name | Type | Description | | ||
| |---|---|---| | ||
| | _0 | uint48 | undefined | | ||
|
|
There was a problem hiding this comment.
Similar to the previous comment, the clock function is also missing a description for its return value.
| _0 | uint48 | undefined |
+152: | _0 | uint48 | Current clock value |
| ### eip712Domain | ||
|
|
||
| ```solidity | ||
| function eip712Domain() external view returns (bytes1 fields, string name, string version, uint256 chainId, address verifyingContract, bytes32 salt, uint256[] extensions) | ||
| ``` | ||
|
|
||
|
|
||
|
|
||
| *See {EIP-5267}. _Available since v4.9._* | ||
|
|
||
|
|
||
| #### Returns | ||
|
|
||
| | Name | Type | Description | | ||
| |---|---|---| | ||
| | fields | bytes1 | undefined | | ||
| | name | string | undefined | | ||
| | version | string | undefined | | ||
| | chainId | uint256 | undefined | | ||
| | verifyingContract | address | undefined | | ||
| | salt | bytes32 | undefined | | ||
| | extensions | uint256[] | undefined | |
There was a problem hiding this comment.
The eip712Domain function is missing descriptions for its return values. Providing these descriptions can help other developers understand what each returned value represents.
| fields | bytes1 | undefined |
| name | string | undefined |
| version | string | undefined |
| chainId | uint256 | undefined |
| verifyingContract | address | undefined |
| salt | bytes32 | undefined |
| extensions | uint256[] | undefined |
+269: | fields | bytes1 | EIP-712 domain struct fields |
+270: | name | string | Name of the EIP-712 domain |
+271: | version | string | Version of the EIP-712 domain |
+272: | chainId | uint256 | Chain ID for the EIP-712 domain |
+273: | verifyingContract | address | Address of the contract that verifies the EIP-712 domain |
+274: | salt | bytes32 | Salt for the EIP-712 domain |
+275: | extensions | uint256[] | Extensions for the EIP-712 domain |
|
|
||
| #### Parameters | ||
|
|
||
| | Name | Type | Description | | ||
| |---|---|---| | ||
| | blockNumber | uint256 | undefined | | ||
| | timepoint | uint256 | undefined | |
There was a problem hiding this comment.
The getPastTotalSupply function is missing a description for its parameter timepoint. This should be added for clarity.
| timepoint | uint256 | undefined |
+291: | timepoint | uint256 | The timepoint at which to retrieve the total supply |
|
|
||
| #### Parameters | ||
|
|
||
| | Name | Type | Description | | ||
| |---|---|---| | ||
| | account | address | undefined | | ||
| | blockNumber | uint256 | undefined | | ||
| | timepoint | uint256 | undefined | |
There was a problem hiding this comment.
Similarly, the getPastVotes function is missing a description for its parameter timepoint.
| timepoint | uint256 | undefined |
+314: | timepoint | uint256 | The timepoint at which to retrieve the votes |
| ### EIP712DomainChanged | ||
|
|
||
| ```solidity | ||
| event EIP712DomainChanged() | ||
| ``` |
There was a problem hiding this comment.
The EIP712DomainChanged event is missing a description. It's important to document what this event signifies.
event EIP712DomainChanged()
+593:
+594: *Triggered when the EIP-712 domain changes.*
| ## Errors | ||
|
|
||
| ### InvalidShortString | ||
|
|
||
| ```solidity | ||
| error InvalidShortString() | ||
| ``` | ||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
| ### StringTooLong | ||
|
|
||
| ```solidity | ||
| error StringTooLong(string str) | ||
| ``` | ||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
| #### Parameters | ||
|
|
||
| | Name | Type | Description | | ||
| |---|---|---| | ||
| | str | string | undefined | | ||
|
|
There was a problem hiding this comment.
The error types InvalidShortString and StringTooLong are missing descriptions. These should be added to explain when these errors are thrown.
error InvalidShortString()
+626:
+627: *Thrown when a short string is invalid.*
error StringTooLong(string str)
+637:
+638: *Thrown when a string exceeds the maximum length.*
| str | string | undefined |
+647: | str | string | The string that is too long |
| @@ -479,7 +479,7 @@ function committeeCheckedIn() external view returns (bool) | |||
| ### convertToAssets | |||
|
|
|||
| ```solidity | |||
| function convertToAssets(uint256 shares) external view returns (uint256 assets) | |||
| function convertToAssets(uint256 shares) external view returns (uint256) | |||
There was a problem hiding this comment.
The function signature has been changed and the explicit variable name in the return type has been removed. This might affect readability of the code, especially for developers who are not familiar with the codebase. However, this is a minor issue and does not affect the functionality of the code.
- function convertToAssets(uint256 shares) external view returns (uint256 assets)
+ function convertToAssets(uint256 shares) external view returns (uint256)
| @@ -496,12 +496,12 @@ function convertToAssets(uint256 shares) external view returns (uint256 assets) | |||
|
|
|||
| | Name | Type | Description | | |||
| |---|---|---| | |||
| | assets | uint256 | undefined | | |||
| | _0 | uint256 | undefined | | |||
There was a problem hiding this comment.
The parameter description in the documentation has been changed from assets to _0. This reduces the readability and understandability of the documentation. It's better to keep meaningful names for parameters in the documentation.
- | assets | uint256 | undefined |
+ | _0 | uint256 | undefined |
|
|
||
| ### convertToShares | ||
|
|
||
| ```solidity | ||
| function convertToShares(uint256 assets) external view returns (uint256 shares) | ||
| function convertToShares(uint256 assets) external view returns (uint256) |
There was a problem hiding this comment.
Similar to the previous comment, the function signature has been changed and the explicit variable name in the return type has been removed. This might affect readability of the code, especially for developers who are not familiar with the codebase. However, this is a minor issue and does not affect the functionality of the code.
- function convertToShares(uint256 assets) external view returns (uint256 shares)
+ function convertToShares(uint256 assets) external view returns (uint256)
| @@ -518,7 +518,7 @@ function convertToShares(uint256 assets) external view returns (uint256 shares) | |||
|
|
|||
| | Name | Type | Description | | |||
| |---|---|---| | |||
| | shares | uint256 | undefined | | |||
| | _0 | uint256 | undefined | | |||
There was a problem hiding this comment.
The parameter description in the documentation has been changed from shares to _0. This reduces the readability and understandability of the documentation. It's better to keep meaningful names for parameters in the documentation.
- | shares | uint256 | undefined |
+ | _0 | uint256 | undefined |
|
|
||
| contract("HATTokenWrapper", (accounts) => { | ||
|
|
||
| let token; | ||
| let tokenWrapper; | ||
|
|
||
| async function setupTokenWrapper() { | ||
| token = await ERC20Mock.new("Staking", "STK"); | ||
| tokenWrapper = await HATTokenWrapper.new(token.address, "Staking Wrapper", "STKW"); | ||
| } |
There was a problem hiding this comment.
The setupTokenWrapper function is used in multiple test cases. It would be better to move it into a beforeEach block to ensure that each test case starts with a fresh instance of the token and token wrapper.
- async function setupTokenWrapper() {
- token = await ERC20Mock.new("Staking", "STK");
- tokenWrapper = await HATTokenWrapper.new(token.address, "Staking Wrapper", "STKW");
- }
+ beforeEach(async () => {
+ token = await ERC20Mock.new("Staking", "STK");
+ tokenWrapper = await HATTokenWrapper.new(token.address, "Staking Wrapper", "STKW");
+ });
|
|
||
|
|
||
| it("minimum amount of shares must be at least MINIMAL_AMOUNT_OF_SHARES", async () => { | ||
| await setupTokenWrapper(); | ||
| const holder = accounts[1]; | ||
| await token.mint(holder, "10000000"); | ||
| await token.approve(tokenWrapper.address, "10000000", { from: holder}); | ||
| // 1e6 is the minimum deposit | ||
| await assertFunctionRaisesException(tokenWrapper.deposit("1", holder, { from: holder }), "AmountOfSharesMustBeMoreThanMinimalAmount"); | ||
| await assertFunctionRaisesException(tokenWrapper.deposit((MINIMAL_AMOUNT_OF_SHARES-1), holder, { from: holder }), "AmountOfSharesMustBeMoreThanMinimalAmount"); | ||
| await tokenWrapper.deposit(MINIMAL_AMOUNT_OF_SHARES, holder, { from: holder }); | ||
|
|
||
| assert.equal((await token.balanceOf(tokenWrapper.address)).toString(), MINIMAL_AMOUNT_OF_SHARES); | ||
|
|
||
| // redeem all the remaining tokens except 1 | ||
| await assertFunctionRaisesException(tokenWrapper.redeem(MINIMAL_AMOUNT_OF_SHARES-1, holder, holder, { from: holder }), "AmountOfSharesMustBeMoreThanMinimalAmount"); | ||
| await tokenWrapper.redeem(MINIMAL_AMOUNT_OF_SHARES, holder, holder, { from: holder }); | ||
| assert.equal((await token.balanceOf(tokenWrapper.address)).toString(), "0"); | ||
| assert.equal((await tokenWrapper.totalSupply()).toString(), "0"); | ||
| }); |
There was a problem hiding this comment.
In the second test case, you are testing both deposit and redeem functions together. It's better to separate these tests for better readability and maintainability. Also, it's good practice to have one assertion per test case. This way, if a test fails, you know exactly what functionality is broken without having to dig through multiple assertions.
- it("minimum amount of shares must be at least MINIMAL_AMOUNT_OF_SHARES", async () => {
+ it("deposit should fail if amount is less than MINIMAL_AMOUNT_OF_SHARES", async () => {
await setupTokenWrapper();
const holder = accounts[1];
await token.mint(holder, "10000000");
await token.approve(tokenWrapper.address, "10000000", { from: holder});
// 1e6 is the minimum deposit
await assertFunctionRaisesException(tokenWrapper.deposit("1", holder, { from: holder }), "AmountOfSharesMustBeMoreThanMinimalAmount");
await assertFunctionRaisesException(tokenWrapper.deposit((MINIMAL_AMOUNT_OF_SHARES-1), holder, { from: holder }), "AmountOfSharesMustBeMoreThanMinimalAmount");
await tokenWrapper.deposit(MINIMAL_AMOUNT_OF_SHARES, holder, { from: holder });
assert.equal((await token.balanceOf(tokenWrapper.address)).toString(), MINIMAL_AMOUNT_OF_SHARES);
- });
- it("redeem should fail if amount is less than MINIMAL_AMOUNT_OF_SHARES", async () => {
+ it("redeem should fail if remaining tokens are less than MINIMAL_AMOUNT_OF_SHARES", async () => {
await setupTokenWrapper();
const holder = accounts[1];
await token.mint(holder, "10000000");
await token.approve(tokenWrapper.address, "10000000", { from: holder});
await tokenWrapper.deposit(MINIMAL_AMOUNT_OF_SHARES, holder, { from: holder });
// redeem all the remaining tokens except 1
await assertFunctionRaisesException(tokenWrapper.redeem(MINIMAL_AMOUNT_OF_SHARES-1, holder, holder, { from: holder }), "AmountOfSharesMustBeMoreThanMinimalAmount");
await tokenWrapper.redeem(MINIMAL_AMOUNT_OF_SHARES, holder, holder, { from: holder });
assert.equal((await token.balanceOf(tokenWrapper.address)).toString(), "0");
assert.equal((await tokenWrapper.totalSupply()).toString(), "0");
});
test/hattokenwrapper.js
Outdated
| assert.equal((await token.balanceOf(tokenWrapper.address)).toString(), "0"); | ||
| assert.equal((await tokenWrapper.totalSupply()).toString(), "0"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Please add a newline at the end of the file to adhere to standard conventions.
- });
+ });
+
| ### CLOCK_MODE | ||
|
|
||
| ```solidity | ||
| function CLOCK_MODE() external view returns (string) | ||
| ``` | ||
|
|
||
|
|
||
|
|
||
| *Description of the clock* | ||
|
|
||
|
|
||
| #### Returns | ||
|
|
||
| | Name | Type | Description | | ||
| |---|---|---| | ||
| | _0 | string | undefined | | ||
|
|
There was a problem hiding this comment.
The CLOCK_MODE method is missing a description for its return value. It's important to provide clear and concise documentation for all public methods, especially for those that are part of an interface.
| _0 | string | undefined |
+26: | _0 | string | The clock mode |
| ### clock | ||
|
|
||
| ```solidity | ||
| function clock() external view returns (uint48) | ||
| ``` | ||
|
|
||
|
|
||
|
|
||
| *Clock used for flagging checkpoints. Can be overridden to implement timestamp based checkpoints (and voting).* | ||
|
|
||
|
|
||
| #### Returns | ||
|
|
||
| | Name | Type | Description | | ||
| |---|---|---| | ||
| | _0 | uint48 | undefined | | ||
|
|
There was a problem hiding this comment.
Similar to the previous comment, the clock method is also missing a description for its return value.
| _0 | uint48 | undefined |
+202: | _0 | uint48 | The current clock value |
| ### eip712Domain | ||
|
|
||
| ```solidity | ||
| function eip712Domain() external view returns (bytes1 fields, string name, string version, uint256 chainId, address verifyingContract, bytes32 salt, uint256[] extensions) | ||
| ``` | ||
|
|
||
|
|
||
|
|
||
| *See {EIP-5267}. _Available since v4.9._* | ||
|
|
||
|
|
||
| #### Returns | ||
|
|
||
| | Name | Type | Description | | ||
| |---|---|---| | ||
| | fields | bytes1 | undefined | | ||
| | name | string | undefined | | ||
| | version | string | undefined | | ||
| | chainId | uint256 | undefined | | ||
| | verifyingContract | address | undefined | | ||
| | salt | bytes32 | undefined | | ||
| | extensions | uint256[] | undefined | | ||
|
|
There was a problem hiding this comment.
The eip712Domain method lacks descriptions for its return values. Providing these descriptions can help other developers understand what each component of the returned tuple represents.
| fields | bytes1 | undefined |
| name | string | undefined |
| version | string | undefined |
| chainId | uint256 | undefined |
| verifyingContract | address | undefined |
| salt | bytes32 | undefined |
| extensions | uint256[] | undefined |
+336: | fields | bytes1 | EIP-712 domain struct fields |
+337: | name | string | Name of the EIP-712 domain |
+338: | version | string | Version of the EIP-712 domain |
+339: | chainId | uint256 | Chain ID for the EIP-712 domain |
+340: | verifyingContract | address | Address of the contract implementing the EIP-712 domain |
+341: | salt | bytes32 | Unique salt value for the EIP-712 domain |
+342: | extensions | uint256[] | Array of extension values for the EIP-712 domain |
| function getPastTotalSupply(uint256 timepoint) external view returns (uint256) | ||
| ``` | ||
|
|
||
|
|
||
|
|
||
| *Retrieve the `totalSupply` at the end of `blockNumber`. Note, this value is the sum of all balances. It is but NOT the sum of all the delegated votes! Requirements: - `blockNumber` must have been already mined* | ||
| *Retrieve the `totalSupply` at the end of `timepoint`. Note, this value is the sum of all balances. It is NOT the sum of all the delegated votes! Requirements: - `timepoint` must be in the past* | ||
|
|
||
| #### Parameters | ||
|
|
||
| | Name | Type | Description | | ||
| |---|---|---| | ||
| | blockNumber | uint256 | undefined | | ||
| | timepoint | uint256 | undefined | |
There was a problem hiding this comment.
The getPastTotalSupply method is missing a description for its parameter timepoint.
| timepoint | uint256 | undefined |
+375: | timepoint | uint256 | The timepoint to retrieve the total supply from |
| function getPastVotes(address account, uint256 timepoint) external view returns (uint256) | ||
| ``` | ||
|
|
||
|
|
||
|
|
||
| *Retrieve the number of votes for `account` at the end of `blockNumber`. Requirements: - `blockNumber` must have been already mined* | ||
| *Retrieve the number of votes for `account` at the end of `timepoint`. Requirements: - `timepoint` must be in the past* | ||
|
|
||
| #### Parameters | ||
|
|
||
| | Name | Type | Description | | ||
| |---|---|---| | ||
| | account | address | undefined | | ||
| | blockNumber | uint256 | undefined | | ||
| | timepoint | uint256 | undefined | |
There was a problem hiding this comment.
The getPastVotes method is missing descriptions for its parameters account and timepoint.
| account | address | undefined |
| timepoint | uint256 | undefined |
+397: | account | address | The address to retrieve the vote count for |
+398: | timepoint | uint256 | The timepoint to retrieve the vote count from |
| ### InvalidShortString | ||
|
|
||
| ```solidity | ||
| error InvalidShortString() | ||
| ``` | ||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
| ### StringTooLong | ||
|
|
||
| ```solidity | ||
| error StringTooLong(string str) | ||
| ``` | ||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
| #### Parameters | ||
|
|
||
| | Name | Type | Description | | ||
| |---|---|---| | ||
| | str | string | undefined | | ||
|
|
There was a problem hiding this comment.
The new error types InvalidShortString and StringTooLong are missing descriptions. It's important to document these errors to help developers understand when they might be thrown.
error InvalidShortString()
+899: error InvalidShortString() "The provided string is not valid for conversion to a short string"
error StringTooLong(string str)
+910: error StringTooLong(string str) "The provided string exceeds the maximum length for a short string"
| str | string | undefined |
+919: | str | string | The string that was too long |
There was a problem hiding this comment.
Files selected (1)
- test/hattokenwrapper.js (1)
Review comments generated (1)
- Review: 1
- LGTM: 0
Tips
Chat with 
CodeRabbit Bot (@coderabbitai)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@coderabbitaiin a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@coderabbitai: ignoreanywhere in the PR description to pause further reviews from the bot.
| const ERC20Mock = artifacts.require("./ERC20Mock.sol"); | ||
| const HATTokenWrapper = artifacts.require("./HATTokenWrapper.sol"); | ||
| const { contract } = require("hardhat"); | ||
| const { | ||
| assertFunctionRaisesException, | ||
| } = require("./common.js"); | ||
| const { assert } = require("chai"); | ||
|
|
||
| const MINIMAL_AMOUNT_OF_SHARES = 1000; | ||
|
|
||
| contract("HATTokenWrapper", (accounts) => { | ||
|
|
||
| let token; | ||
| let tokenWrapper; | ||
|
|
||
| async function setupTokenWrapper() { | ||
| token = await ERC20Mock.new("Staking", "STK"); | ||
| tokenWrapper = await HATTokenWrapper.new(token.address, "Staking Wrapper", "STKW"); | ||
| } | ||
|
|
||
| it("correct init", async () => { | ||
| await setupTokenWrapper(); | ||
|
|
||
| assert.equal(await tokenWrapper.name(), "Staking Wrapper"); | ||
| assert.equal(await tokenWrapper.symbol(), "STKW"); | ||
| assert.equal(await tokenWrapper.asset(), token.address); | ||
| }); | ||
|
|
||
|
|
||
| it("minimum amount of shares must be at least MINIMAL_AMOUNT_OF_SHARES", async () => { | ||
| await setupTokenWrapper(); | ||
| const holder = accounts[1]; | ||
| await token.mint(holder, "10000000"); | ||
| await token.approve(tokenWrapper.address, "10000000", { from: holder}); | ||
| // 1e6 is the minimum deposit | ||
| await assertFunctionRaisesException(tokenWrapper.deposit("1", holder, { from: holder }), "AmountOfSharesMustBeMoreThanMinimalAmount"); | ||
| await assertFunctionRaisesException(tokenWrapper.deposit((MINIMAL_AMOUNT_OF_SHARES-1), holder, { from: holder }), "AmountOfSharesMustBeMoreThanMinimalAmount"); | ||
| await tokenWrapper.deposit(MINIMAL_AMOUNT_OF_SHARES, holder, { from: holder }); | ||
|
|
||
| assert.equal((await token.balanceOf(tokenWrapper.address)).toString(), MINIMAL_AMOUNT_OF_SHARES); | ||
|
|
||
| // redeem all the remaining tokens except 1 | ||
| await assertFunctionRaisesException(tokenWrapper.redeem(MINIMAL_AMOUNT_OF_SHARES-1, holder, holder, { from: holder }), "AmountOfSharesMustBeMoreThanMinimalAmount"); | ||
| await tokenWrapper.redeem(MINIMAL_AMOUNT_OF_SHARES, holder, holder, { from: holder }); | ||
| assert.equal((await token.balanceOf(tokenWrapper.address)).toString(), "0"); | ||
| assert.equal((await tokenWrapper.totalSupply()).toString(), "0"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The test cases provided in the HATTokenWrapper contract seem to cover the basic functionality of the contract. However, there are a few additional scenarios that could be tested to ensure robustness:
- What happens when someone tries to deposit or redeem tokens without having enough balance?
- What happens if someone tries to deposit or redeem tokens without first approving the
HATTokenWrappercontract? - Are there any edge cases around the
MINIMAL_AMOUNT_OF_SHARESconstant? For example, what happens if someone tries to deposit exactlyMINIMAL_AMOUNT_OF_SHAREStokens?
Consider adding tests for these scenarios to ensure the contract behaves as expected in all situations.
Also, it would be beneficial to add some comments explaining what each test case is doing and why. This will make it easier for other developers to understand the purpose of each test and maintain them in the future.
Lastly, consider using a testing framework like Mocha or Jest which provides a more structured way to organize your tests and gives you access to hooks like beforeEach where you can put the setup code for your tests. This would eliminate the need to call setupTokenWrapper() at the beginning of each test case.
Here's an example of how you might structure your tests with Mocha:
describe("HATTokenWrapper", function() {
let token;
let tokenWrapper;
beforeEach(async function() {
token = await ERC20Mock.new("Staking", "STK");
tokenWrapper = await HATTokenWrapper.new(token.address, "Staking Wrapper", "STKW");
});
it("correct init", async function() {
assert.equal(await tokenWrapper.name(), "Staking Wrapper");
assert.equal(await tokenWrapper.symbol(), "STKW");
assert.equal(await tokenWrapper.asset(), token.address);
});
// ...other tests...
});
Add token wrapper of ERC4626 for rebasing tokens to use the Hats vaults
Update OZ to 4.9.3
Summary by CodeRabbit
New Features:
HATTokenWrappercontract for rebasing tokens using Hats vaults.IERC5805contract with voting delegation methods and events.Refactor:
Documentation: