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

Add EIP: Redeemable Tokens #6732

Closed
wants to merge 20 commits into from
Closed

Add EIP: Redeemable Tokens #6732

wants to merge 20 commits into from

Conversation

lucazffz
Copy link

@lucazffz lucazffz commented Mar 20, 2023

A proposal for redeemable tokens.

@lucazffz lucazffz requested a review from eth-bot as a code owner March 20, 2023 19:29
@github-actions github-actions bot added c-new Creates a brand new proposal s-draft This EIP is a Draft t-erc labels Mar 20, 2023
@eth-bot
Copy link
Collaborator

eth-bot commented Mar 20, 2023

File EIPS/eip-6732.md

Requires 1 more reviewers from @axic, @Pandapip1, @SamWilsn, @xinbenlv

@eth-bot eth-bot added e-consensus Waiting on editor consensus e-review Waiting on editor to review labels Mar 20, 2023
@github-actions github-actions bot added the w-ci Waiting on CI to pass label Mar 20, 2023
@abcoathup
Copy link
Contributor

abcoathup commented Mar 21, 2023

EIP editors assign an EIP number (generally the PR number, but the decision is with the editors) (from: EIP1)

I couldn't see where an EIP editor assigned ERC4365.

Earlier #6731 PR was closed

@lucazffz lucazffz closed this Mar 21, 2023
@lucazffz lucazffz reopened this Mar 21, 2023
@lucazffz
Copy link
Author

@abcoathup Oh okay, I must have missed that. I apologize. I will remove the EIP number. However, it would be easier to keep this number since it's not taken and used extensively in the reference implementation code. I can absolutely change that if necessary.

@lucazffz lucazffz changed the title Add EIP-4365: Redeemable Tokens Add EIP-X: Redeemable Tokens Mar 21, 2023
@github-actions github-actions bot added the e-number Waiting on EIP Number assignment label Mar 21, 2023
@lucazffz lucazffz changed the title Add EIP-X: Redeemable Tokens Add Redeemable Tokens Mar 21, 2023
@lucazffz lucazffz changed the title Add Redeemable Tokens Add eip-draft_title_abbrev.md: Redeemable Tokens Mar 21, 2023
@lucazffz lucazffz changed the title Add eip-draft_title_abbrev.md: Redeemable Tokens Add eip-draft_title_abbrev: Redeemable Tokens Mar 21, 2023
@github-actions github-actions bot removed the w-ci Waiting on CI to pass label Mar 21, 2023
@lucazffz lucazffz changed the title Add eip-draft_title_abbrev: Redeemable Tokens Add Redeemable Tokens Mar 21, 2023
@github-actions github-actions bot added w-ci Waiting on CI to pass and removed c-new Creates a brand new proposal e-number Waiting on EIP Number assignment labels Mar 21, 2023
Copy link
Member

@Pandapip1 Pandapip1 left a comment

Choose a reason for hiding this comment

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

Please fix the CI errors.

We're experimenting with a new automated system to assign EIP numbers. Feel free to ignore the eip walidator error about the missing eip header for now.

Avoid self-references. Simply refer to the current eip as "this EIP", rather than by "EIP-X". For the code, simply use descriptive names for the classes. It's much more readable than an opaque number, anyway.

@eth-bot eth-bot changed the title Add Redeemable Tokens Add EIP: Redeemable Tokens Mar 22, 2023
Copy link
Member

@Pandapip1 Pandapip1 left a comment

Choose a reason for hiding this comment

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

You get the drill.

EIPS/eip-draft_title_abbrev.md Outdated Show resolved Hide resolved
EIPS/eip-draft_title_abbrev.md Outdated Show resolved Hide resolved
EIPS/eip-draft_title_abbrev.md Outdated Show resolved Hide resolved
EIPS/eip-draft_title_abbrev.md Outdated Show resolved Hide resolved
@github-actions github-actions bot added the c-new Creates a brand new proposal label Mar 24, 2023
mspaansen
mspaansen previously approved these changes Mar 25, 2023
@lucazffz lucazffz changed the title Add EIP: Redeemable Tokens Add EIP-6732: Redeemable Tokens Mar 28, 2023
@github-actions
Copy link

The commit 4699fd2 (as a parent of 1a84d4a) contains errors.
Please inspect the Run Summary for details.

@github-actions github-actions bot removed the w-ci Waiting on CI to pass label Mar 28, 2023
@Pandapip1
Copy link
Member

@eth-bot rerun

@eth-bot eth-bot changed the title Add EIP-6732: Redeemable Tokens Add EIP: Redeemable Tokens Mar 29, 2023
Copy link
Contributor

@SamWilsn SamWilsn left a comment

Choose a reason for hiding this comment

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

In addition to my other comments, would you mind reducing your reference implementation down to something more minimal? At minimum you should remove any of the OZ source. The readers can find those implementations themselves.

Generally the reference implementation should be one or two files to illustrate your proposal, and not a fully working production contract.

EIPS/eip-6732.md Outdated Show resolved Hide resolved
EIPS/eip-6732.md Show resolved Hide resolved
EIPS/eip-6732.md Outdated Show resolved Hide resolved
EIPS/eip-6732.md Outdated Show resolved Hide resolved
EIPS/eip-6732.md Outdated

## Reference Implementation

- [Reference implementation](../assets/eip-6732/)
Copy link
Contributor

Choose a reason for hiding this comment

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

The markdown renderer probably won't let you link to the whole directory. We normally recommend linking to the readme.

Copy link
Author

@lucazffz lucazffz Apr 20, 2023

Choose a reason for hiding this comment

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

Do you mean the reference implementation's readme? If I should remove the migrations and slim the implementation down I suppose I should also remove the reference implementation's readme since it's not necessary at that point. Correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, if you cram the whole reference implementation into a single file, just link to that. If not, you can make a README.md that's just a manual listing of files:

# ERC-XXXX Reference Implementation

 * [SomeContract.sol](./SomeContract.sol)
 * [MagicInterface.sol](./MagicInterface.sol)

truffle deploy
```

However, dont forget to link Ganache to the project (tutorial [here](https://trufflesuite.com/docs/ganache/how-to/link-a-truffle-project/)), alternatively create a Ganache quickstart workspace and match the server host and port in `truffle-config.js`.
Copy link
Contributor

Choose a reason for hiding this comment

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

You'll need to remove external links from your reference implementation as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this file please.

Copy link
Contributor

Choose a reason for hiding this comment

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

The reference implementation shouldn't need migrations. It shouldn't be a fully working project, just an illustrative example without any extra code.

Copy link
Author

Choose a reason for hiding this comment

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

Would it be sufficient to keep the files under the examples and token folders and remove the rest including utils and migrations?

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds reasonable, yes.

Copy link
Contributor

@SamWilsn SamWilsn left a comment

Choose a reason for hiding this comment

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

You'll also still need to remove external links and build files from your reference implementation.


- Implementers SHOULD NOT allow tokens to be minted if total supply exceeds max supply.
- Implementers SHOULD increment total supply upon minting and decrement upon burning.
- Implementers are RECOMMENDED to override the `_beforeMint` hook to increment total supply upon minting and decrement upon burning.
Copy link
Contributor

Choose a reason for hiding this comment

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

The _beforeMint function is part of the reference implementation, not the specification itself. The Specification section should be understandable on its own without the other sections, while the reference implementation should help to clarify what's in the the Specification section.

Suggested change
- Implementers are RECOMMENDED to override the `_beforeMint` hook to increment total supply upon minting and decrement upon burning.

@github-actions
Copy link

There has been no activity on this pull request for 2 weeks. It will be closed after 3 months of inactivity. If you would like to move this PR forward, please respond to any outstanding feedback or add a comment indicating that you have addressed all required feedback and are ready for a review.

@github-actions github-actions bot added the w-stale Waiting on activity label Sep 28, 2023
@SamWilsn
Copy link
Contributor

I am closing this pull request because we are in the process of separating EIPs and ERCs into distinct repositories. Unfortunately, as far as we are aware, GitHub does not provide any tools to ease this migration, so every pull request will need to be re-opened manually.

As this is a PR to create / modify an ERC, I will kindly ask you to redirect this to the new repository at ethereum/ERCs. We have prepared a guide to help with the process.

If there is relevant history here, please link to this PR from the new pull request.

On behalf of the EIP Editors, I apologize for this inconvenience.

@SamWilsn SamWilsn closed this Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-new Creates a brand new proposal e-consensus Waiting on editor consensus e-review Waiting on editor to review s-draft This EIP is a Draft t-erc w-stale Waiting on activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants