Skip to content
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

feat: Non transferable token extensions #692

Conversation

samuel-videau
Copy link
Contributor

What does this PR introduce?

🚀 Feature

This PR introduce an extension to make tokens (LSP7 or LSP8) non-transferable.

In these extensions, the _transfer functions are overriten to revert if called.
A flag (as a ERC725Y key) is added, in order to let application/smart contracts know if they are dealing with a non-transferable token. This flag can't be edited after contract deployment or initialization.

Thoughts:

  • 2 keys were added for the flags (LSP7NonTransferable & LSP8NonTransferable), though it might be interesting having a common "LSPNonTransferable" key that would be shared by any token standard that want to implement a non-transferable logic.
  • Should we also override functions like authorizeOperator ?

PR Checklist

  • Wrote Tests
  • Wrote & Generated Documentation (readme/natspec/dodoc)
  • Ran npm run lint && npm run lint:solidity (solhint)
  • Ran npm run format (prettier)
  • Ran npm run build
  • Ran npm run test

@@ -180,6 +180,14 @@ export const ERC725YDataKeys = {
LSP9: {
SupportedStandards_LSP9: SupportedStandards.LSP9Vault.key,
},
LSP7: {
// keccak256('LSP7NonTransferable')
LSP7NonTransferable: '0xb48a0085fb2c841c614d8c650033c5b790f407c071b9176ca4f2003db3613a1f',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would have these data keys first. I would suggest opening an issue in the LIP repo to discuss this first.

@YamenMerhi @skimaharvey @b00ste
https://github.com/lukso-network/LIPs/issues

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue created: lukso-network/LIPs#231

) internal virtual override {
if (dataKey == _LSP7_NON_TRANSFERABLE) {
revert LSP7NonTransferableNotEditable();
} else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the else is not needed here. But not a big deal.


/**
* @dev the ERC725Y data key `LSP7NonTransferable` cannot be changed
* via this function once the digital asset contract has been deployed.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the indent here in the Natspec comments, so that they can be parsed correctly in the auto-generated docs in docs.lukso.tech

Suggested change
* via this function once the digital asset contract has been deployed.
* via this function once the digital asset contract has been deployed.

bytes memory data
) internal virtual override {
revert("LSP7: Token is non-transferable");
from;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please comment the variable names instead to silence the solc compiler warning. Like this:

    function _transfer(
        address /* from */,
        address /* to */,
        uint256 /* amount */,
        bool /* allowNonLSP1Recipient */,
        bytes memory /* data */
    ) internal virtual override {
        revert("LSP7: Token is non-transferable");
    }

}

/**
* @notice This function override _transfer function to make it non-transferable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @notice This function override _transfer function to make it non-transferable
* @dev This function override _transfer function to make it non-transferable

/**
* @notice Deploying a `LSP7NonTransferable` token contract with: token name = `name_`, token symbol = `symbol_`,
* address `newOwner_` as the token contract owner, and _isNonDivisible_ = `isNonDivisible_`.
* Set the non-transferable flag.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small suggestion

Suggested change
* Set the non-transferable flag.
* Switch ON the non-transferable flag.

* @param name_ The name of the token.
* @param symbol_ The symbol of the token.
* @param newOwner_ The owner of the token contract.
* @param isNonDivisible_ Specify if the LSP7 token is a fungible or non-fungible token
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not correct. If this parameter is true, the token is still a fungible token.

Suggested change
* @param isNonDivisible_ Specify if the LSP7 token is a fungible or non-fungible token
* @param isNonDivisible_ Specify if the tokens from this contract can be divided in smaller units or not.

}

/**
* @notice This function override _transfer function to make it non-transferable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @notice This function override _transfer function to make it non-transferable
* @notice This function override the internal `_transfer` function to make it non-transferable

* @param name_ The name of the token.
* @param symbol_ The symbol of the token.
* @param newOwner_ The owner of the token contract.
* @param isNonDivisible_ Specify if the LSP7 token is a fungible or non-fungible token
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as in the other file.

@CJ42
Copy link
Member

CJ42 commented Jul 26, 2024

Closing this PR as similar to #804 and #805

We need more inputs and clarify more the roadmap on the Token extension for tokens and NFTs. PR could be re-opened tho in the future if we consider this is relevant.

Also this feature might not require a token extension, as it can be easily done by overriding the internal _transfer function.

@CJ42 CJ42 closed this Jul 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants