-
Notifications
You must be signed in to change notification settings - Fork 50
feat: create customizeable token contract #1018
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
base: main
Are you sure you want to change the base?
Conversation
9034a3c
to
71f51dc
Compare
a7407c0
to
53e8785
Compare
@b00ste we also need to allow developers to import these new extensions from the Lines 1 to 4 in 8d4fef1
|
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.
Added first round of review. It is already much better with the contracts broken down in multiple inheritance. Also great Natspec comments overall and well documented 👏🏻
@@ -5,7 +5,7 @@ libs = ['node_modules','lib'] | |||
gas_reports = ["LSP6ExecuteRestrictedController", "LSP6ExecuteUnrestrictedController", "LSP6SetDataRestrictedController", "LSP6SetDataUnrestrictedController"] | |||
cache_path = 'forge-cache' | |||
test = 'tests/foundry' | |||
solc = "0.8.17" | |||
solc = "0.8.22" |
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 since we are using the feature from 0.8.22 `(to access events inside contracts), it will not compile with Hardhat for these new token extensions no? (Since Hardhat defines 0.8.17 version
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 if you're using hardhat compile, it will use the version defined in hardhat config
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.
pragma solidity ^0.8.4; | ||
|
||
/// @title ILSP7Allowlist | ||
/// @dev Interface for managing an allowlist of addresses that can bypass certain restrictions in an LSP7 token contract. |
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.
What is meant by "bypass" certain restrictions here? Maybe we could put some examples?
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 see there is an example below. Could we put other examples to illustrate more the use case of this token extension?
packages/lsp7-contracts/contracts/customizable/ILSP7Allowlist.sol
Outdated
Show resolved
Hide resolved
packages/lsp7-contracts/contracts/customizable/ILSP7Allowlist.sol
Outdated
Show resolved
Hide resolved
packages/lsp7-contracts/contracts/customizable/LSP7AllowlistAbstract.sol
Show resolved
Hide resolved
} | ||
|
||
/// @inheritdoc ILSP7Mintable | ||
function mint( |
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 would discuss this public function, as there could be any public function for minting. Maybe we should just keep the public mint(...)
function only for the preset and keep only the internal _mint(...)
function inside this token extension contract?
uint256 transferLockStart_, | ||
uint256 transferLockEnd_ | ||
) { | ||
if (transferLockEnd_ < transferLockStart_) { |
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.
What would be the behaviour if transferLockEnd_
and transferLockStart_
are the same value being set?
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.
There will be no transfer lock period
packages/lsp7-contracts/contracts/customizable/LSP7NonTransferableAbstract.sol
Outdated
Show resolved
Hide resolved
packages/lsp7-contracts/contracts/customizable/LSP7NonTransferableAbstract.sol
Outdated
Show resolved
Hide resolved
53e8785
to
0b03da2
Compare
Just came across with this and i like the idea of a customizable token. I will propose ways to work with the allow list (merkle tree or signature method) and batch add/removes |
c4af453
to
e61fd62
Compare
@@ -0,0 +1,279 @@ | |||
// SPDX-License-Identifier: Apache-2.0 | |||
pragma solidity ^0.8.4; |
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.
@@ -5,7 +5,7 @@ libs = ['node_modules','lib'] | |||
gas_reports = ["LSP6ExecuteRestrictedController", "LSP6ExecuteUnrestrictedController", "LSP6SetDataRestrictedController", "LSP6SetDataUnrestrictedController"] | |||
cache_path = 'forge-cache' | |||
test = 'tests/foundry' | |||
solc = "0.8.17" | |||
solc = "0.8.22" |
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.
) internal virtual { | ||
// Allow burning | ||
if (to == address(0)) return; | ||
if (balanceOf(to) + amount <= tokenBalanceCap()) return; |
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.
Ok for the early return statement. Let's then add some extra parameters to the custom error to improve logging
/// @param from The address sending the tokens (ignored in this implementation). | ||
/// @param to The address receiving the tokens. | ||
/// @param amount The amount of tokens being transferred. | ||
/// @param force Whether to force the transfer (ignored in this implementation). |
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.
btw i didn't comment out the params because natspec errors out with them commented
Yes good point for the Natspec compiler error 🤔
What does this PR introduce?
PR Checklist
npm run lint
&&npm run lint:solidity
(solhint)npm run format
(prettier)npm run build
npm run test