Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
draft: LIP-27 - Token Bound Profiles Proposal #304
base: main
Are you sure you want to change the base?
draft: LIP-27 - Token Bound Profiles Proposal #304
Changes from 7 commits
de11e54
f3a3c4b
88e8cd0
4a82d29
5a14787
582f86c
de70618
96a93f2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
So the
owner()
of a UP is the holder of the NFT? What happen if I want my UP to be owned by a KeyManager?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 this can be done with LSP8 using the tokenIdFormat address.
A custom implementation of LSP8 (within the Solidity code, for instance via the
_afterTokenTransfer(...)
hook could: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.
Is there a link to a repo where we can see the Solidity code of this custom 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.
Why having the chainId here? This does not seem necessary. A UP exists only on a specific chain.
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 use LSP8TokenIdFormat = Address + CREATE2, the tokenId would be the address of the UP. So this tokenId parameter would not be necessary. The parameter
address profile
emitted in the event would act at the same time as the tokenId.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.
It should be precised in the standard if CREATE2 opcode MUST or COULD be used.
Meaning if it's mandatory to deploy UPs using CREATE2 or not
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.
This function expects a parameter. What would be the tokenId passed here when calling the
owner()
function? Where is the tokenId stored in the UP? I see it in the constructor but nothing else is mentioned about that.Then this would mean if you want to have this exact behaviour, you would need to:
Override the _acceptOwnership function to call the LSP8 contract to transfer the tokenId to the new owner (challenging as here the caller would be the UP, so some custom logic would need to be implemented in your LSP8 version).
Override the
transfer(...)
function in LSP8 to also transfer ownership of the UP to the new owner. This would not work neither, the call might fail as the caller would not be the tokenId owner but the tokenContract. Therefore more custom logic would need to be implemented on the UP side for this.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.
This function expects a parameter. What would be the tokenId passed here when calling the
owner()
function? Where is the tokenId stored in the UP? I see it in the constructor but nothing else is mentioned about that.Then this would mean if you want to have this exact behaviour, you would need to:
Override the _acceptOwnership function to call the LSP8 contract to transfer the tokenId to the new owner (challenging as here the caller would be the UP, so some custom logic would need to be implemented in your LSP8 version).
Override the
transfer(...)
function in LSP8 to also transfer ownership of the UP to the new owner. This would not work neither, the call might fail as the caller would not be the tokenId owner but the tokenContract. Therefore more custom logic would need to be implemented on the UP side for this.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.
But you would still need to update the
_owner
in the state as well. How would that become different? Otherwise, this would override the standard behaviour (which is from LSP14 Ownable 2 Steps standard that LSP0 uses, so making it less compliant)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 do not find this section clear. This is an important one for proposing a new standard.
It does highlight generically the potential benefits (although I find these more generic statements than specific advantages or problems being solved by the standard).
I would suggest to review this section carefully if looking to push this standard, as the rationale is what motivate the developers and projects in the ecosystem to adopt the standard, as it solves their specific needs.
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 is a nuance here. By doing so, UP would not be able to be held by custom owner contracts (a Key Manager, a Multi sig, etc...).
This would be very restrictive, as as a result, Token Bound profile would not be able to benefit of gas less transactions.
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.
This is a very strong statement. By the look of the function section above, it seems to break partially LSP14 which is part of LSP0. Although this could be debatable to the extent.
The bigger problem is that this standard proposal breaks composability over "who can be the owner of the UP". In the sense "control the UP". Ownership and control (via the
owner()
function) are two very different thingsThere 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.
Yes that's correct from the public interface point of view. So this statement nuances my previous comment above.
However, it does disrupt the standard functionalities.
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.
Interesting section 👀
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.
This must be enforced at the smart contract level. But this my not be applicable as I believe LSP14 Ownable 2 Steps mention in the specs that it cannot transfer ownership to self (to be verified)