Add support for burnable to ERC20, ERC721, and ERC1155 #540
Conversation
src/contracts/nft-drop.ts
Outdated
| return { | ||
| receipt: await this.contractWrapper.sendTransaction("burn", [tokenId]), | ||
| }; | ||
| public async burnFromSelf(tokenId: BigNumberish): Promise<TransactionResult> { |
There was a problem hiding this comment.
let's call this burnToken(tokenId) reads much nicer
src/core/classes/erc-721-burnable.ts
Outdated
| * await contract.nft.burn.fromSelf(tokenId); | ||
| * ``` | ||
| */ | ||
| public async fromSelf(tokenId: BigNumberish): Promise<TransactionResult> { |
There was a problem hiding this comment.
let's also call this token(tokenId)
so it goes contract.nft.burn.token(tokenId)
| @@ -270,11 +268,6 @@ export class Token extends Erc20<TokenERC20> { | |||
| holder: string, | |||
There was a problem hiding this comment.
let's get rid of this function, it's just confusing and has almost no value
There was a problem hiding this comment.
Feels odd to remove this (assuming you're referring to burnFrom because its the only function on the token interface that calls the burnFrom function on the contract: https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/ad67d48f0914a15a9350ef42672322058b48d5b1/contracts/token/ERC20/extensions/ERC20BurnableUpgradeable.sol#L41
Also, it's existed on our token contract for a long time now. We should probably have contract interface for the functions on the contract right? Otherwise just shouldn't be on the contract if it really has no use (but since OZ has it, maybe people use it for something).
There was a problem hiding this comment.
Will delete anyway for now
src/contracts/edition.ts
Outdated
| amount, | ||
| ]), | ||
| }; | ||
| return this._burn.fromSelf(tokenId, amount); |
There was a problem hiding this comment.
this one is still fromSelf ?
src/contracts/token-drop.ts
Outdated
| * await contract.burnFrom(holderAddress, amount); | ||
| * ``` | ||
| */ | ||
| public async burnFrom( |
There was a problem hiding this comment.
thinking more about it, maybe we leave this just in case, never know what weird usecases ppl have. Lets' leave this but not expose it in the extension.
There was a problem hiding this comment.
There's only two functions in the extension though - burn and burnFrom. Isn't it odd to only support one of them if someone intentionally implemented?
| amount: Amount, | ||
| ): Promise<TransactionResult> { | ||
| return { | ||
| receipt: await this.contractWrapper.sendTransaction("burnFrom", [ |
src/contracts/token.ts
Outdated
| ]), | ||
| }; | ||
| public async burnTokens(amount: Amount): Promise<TransactionResult> { | ||
| return this._burn.fromSelf(amount); |
There was a problem hiding this comment.
still called fromSelf here
No description provided.