EIP-4931 - Generic Token Upgrade Standard #4931
Conversation
All tests passed; auto-merging...(pass) eip-4931.md
|
|
|
||
| #### upgrade | ||
|
|
||
| Upgrades the `amount` of source token to the destination token in the specified ratio. The destination tokens will be sent to the ```_to``` address. The function should lock the source tokens in the upgrade contract or burn them. If the downgrade Optional Ext. is implemented, the source tokens should be locked instead of burning. The function should `throw` if the caller's address does not have enough source token to upgrade or if ```isUpgradeActive``` is returning ```false```. The function MUST also fire the `Upgrade` event. `approve` Must be called first on the source contract. |
There was a problem hiding this comment.
If the downgrade Optional Ext. is implemented, the source tokens should be locked instead of burning.
I think this is inappropriate behavior as it creates an asymmetry. Ideally, the sum of totalSupply on both tokens will equal the total supply of the overall upgradable token. This holds for burnable tokens that are upgradable but not downgradable, but it does not hold for burnable tokens which are upgradable and downgradable.
I think the correct behavior here is to always burn when possible (side note: burning should be defined here as adjusting the total supply down).
A mechanism for identifying whether a given token is burnable or not I think should also be included as it allows the person looking at the contract to determine whether or not they can rely on totalSupply summing up to something reasonable or not, and the specification should assert that if a token is burnable, tokens MUST be burned (rather than SHOULD be burned) and it should also strongly recommend that tokens be burnable whenever possible (SHOULD).
On a similar note, it should be more strict about transferring to 0 address and assert that if the token is not burnable, then it MUST transfer to 0. This way people interacting with the token can calculate the total supply across upgrades reliably, regardless of whether it is burnable or not.
There was a problem hiding this comment.
I suppose one issue with the above is that if a source token already exists and isn't mintable, and you want downgrades, then you need the tokens to remain accessible by the upgrader contract. I think this convinces me that the current recommendations are acceptable, but I think they should be made strict with MUST rather than SHOULD.
I think it may actually be best to just assert that the source token is always locked in the upgrade contract, so the behavior is consistent. The problem of course is that totalSupply will be wrong in that case which is unfortunate. Maybe there can be a method exposed that indicates whether total supply is source.totalSupply + destination.totalSupply or source.totalSupply + destination.totalSupply - balenceOf(upgrader)? Or perhaps the upgrader should just be required to expose a totalSupply function that can have whatever logic it wants in it?
There was a problem hiding this comment.
I agree that the language surrounding this should be more strict (SHOULD -> MUST)! I will go ahead and commit this change. As to the proposed assertion that the source token is always to be locked in the upgrade contract, I agree that it would make behavior more consistent but opens up several issues with strictness as well as total supply as you mentioned. We will need to discuss the implications of this further before making that change.
John-peterson-coinbase
changed the title
|
Thanks @Pandapip1 for the comments. I addressed them all in my fork, see cygnusv@8484572. |
|
@John-peterson-coinbase might need to close/reopen this PR to get it to merge |
John-peterson-coinbase
changed the title
@SamWilsn If I close and reopen as suggested, are we able to keep the same EIP number (4931)? |
|
Yes. Just click the "close PR" button and immediately click the "reopen PR" button. |
* EIP - Generic Token Upgrade Standard * Add PR link to discussions-to section * Formatting changes / inline reference implementation * remove external links * Add eip number (4931) and rename md file / reformat reference * Add github usernames for authors * more strict enforcement of source token handling during upgrades * Add David's github username to author list * Add link to ethereum magicians forum post * Correct format and language. * ERC20 link nit in abstract Co-authored-by: David Núñez <david@nucypher.com>
* EIP - Generic Token Upgrade Standard * Add PR link to discussions-to section * Formatting changes / inline reference implementation * remove external links * Add eip number (4931) and rename md file / reformat reference * Add github usernames for authors * more strict enforcement of source token handling during upgrades * Add David's github username to author list * Add link to ethereum magicians forum post * Correct format and language. * ERC20 link nit in abstract Co-authored-by: David Núñez <david@nucypher.com>
Create a standard interface for upgrading ERC20 token contracts.
When opening a pull request to submit a new EIP, please use the suggested template: https://github.com/ethereum/EIPs/blob/master/eip-template.md
We have a GitHub bot that automatically merges some PRs. It will merge yours immediately if certain criteria are met: