fix(contract-standards): remove mint function from exposed nft standard and trait
#525
Conversation
austinabell
requested review from
ChaoticTempest,
evgenykuzyakov and
mikedotexe
as code owners
|
Actually, this is somewhat of a bigger issue, the mint function is exposed as part of the API of any contract which uses the standard, when it really shouldn't be. I'm wondering why mint is part of the trait. |
austinabell
changed the title
mint function from exposed nft standard and trait
| /// * token_id must be unique | ||
| /// | ||
| /// Returns the newly minted token | ||
| pub fn mint( |
There was a problem hiding this comment.
still need to mark it as payable i think
There was a problem hiding this comment.
actually, this is doing the opposite, which is not exposing this function for external calls at all.
Looking at this again, I think this function can just be removed altogether, the mint functionality can still be called through the NonFungibleToken as the NFT example does. I was just trying to mitigate breaking changes but I think this just causes more issues for no benefit.
Any objections from removing this function from the NFT trait which is not part of the standard and this no longer by default exposes the mint function which is currently broken anyway on its current version? cc @mikedotexe
There was a problem hiding this comment.
got your point.
still prefer to keep this function, mark it as payable (other mechanism shall be used to make it internal), in order to avoid any breaking changes. But if you guys are ok to remove this function totally I'm good with it.
There was a problem hiding this comment.
got your point.
still prefer to keep this function, mark it as payable (other mechanism shall be used to make it internal), in order to avoid any breaking changes. But if you guys are ok to remove this function totally I'm good with it.
well, the only breaking change is no longer externally exposing a function that was broken and should not have been included in the standard in the first place. You can still use the mint functionality as the example does, too. I'll wait on @mikedotexe's take or maybe @chadoh since he worked on the standard also.
|
Hey, moving mint function out of the core trait looks good to me! |
Seems like the nft example uses a wrapper definition of mint which is marked as payable, meaning the base impl function is broken. Going to dig a bit deeper into this, but opening for expediency to make sure if wrong that it makes it in for the next release.Hides
mintfunction from being exposed and removes it from theNonFungibleTokenCoretrait, as it is not part of the standard.Closes #522