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

CIP-0025 | Restrict to minting (positive amounts) #527

Merged
merged 1 commit into from
Aug 1, 2023

Conversation

nielstron
Copy link
Contributor

As discussed in #392 (comment) there is a flaw in the design of CIP 25 should new token minting policies make burning permissionless.

In particular, the CIP mentions the latest minting transaction of a token to be the most up-to-date version of the metadata of a token. Burning tokens is not explicitly excluded from being a minting transaction (where technically both minting and burning are minting operations for the ledger, just with positive/negative amounts). If burning would become permisionless, users can arbitrarily change the metadata of their tokens. Hence this change proposes to explicitly forbid burning transactions to be counted for CIP 25 updates.

@nielstron nielstron changed the title Restrict CIP 25 to minting (positive amounts) CIP 25 | Restrict to minting (positive amounts) Jun 29, 2023
@Ryun1 Ryun1 added Update Adds content or significantly reworks an existing proposal Category: Tokens Proposals belonging to the 'Tokens' category. labels Jun 29, 2023
@Ryun1 Ryun1 changed the title CIP 25 | Restrict to minting (positive amounts) CIP-0025 | Restrict to minting (positive amounts) Jun 29, 2023
Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

In my recollection I don't think this issue was considered when CIP-0025 was being debated in its initial review #85. That was around the time I began activity as a CIP editor so I probably missed it if mentioned at a CIP meeting. Otherwise my guess is that the possibility of burns allowing token metadata updates was a side effect not considered at the time.

So @alessandrokonrad @SmaugPool @KtorZ @Crypto2099 unless there are any reservations upon 2 years of reflection, I think this should be updated as "something that should have been there in the first place"... any ideas on that? Everyone knows CIP25 is not intended to be perfect or future proof but it would be nice to have this clarification so #392 can proceed (cc @nielstron).

@KtorZ @Ryun1 @SebastienGllmt I guess we should also poll the community to find out if this addition to CIP-0025 could possibly break anything. If it could, we should introduce a new version for the CIP as per our discussions & learning experience of #520.

@Crypto2099
Copy link
Collaborator

I believe this is the way most of our community tooling has already operated and this seems a very minor clerical point of order so yeah, I'm totally onboard as we shouldn't consider any update to a token during a full or partial burn (negative mint) operation.

Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

Thanks @Crypto2099 for the confirmation - I was suspecting and hoping as such.

Copy link
Collaborator

@Crypto2099 Crypto2099 left a comment

Choose a reason for hiding this comment

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

This is a relatively minor change and I believe the way that our community tooling already operated under "common sense" principles, but it is good to have it officially canonized.

@Ryun1
Copy link
Collaborator

Ryun1 commented Jul 9, 2023

@rphair

I guess we should also poll the community to find out if this addition to CIP-0025 could possibly break anything. If it could, we should introduce a new version for the CIP as per our discussions & learning experience of #520.

Id much more be in favour of a new version of CIP-25, as I am worried about breaking things in a similar manor to what happened with Issue #520. But then again I'm not familiar enough with such implementations to be able to estimate the impact of this change too accurately, so perhaps Im being too cautious 🤓.

@rphair
Copy link
Collaborator

rphair commented Jul 9, 2023

@mmahut you made the report a few weeks ago that adding the 444 implementation as per #494 broke something on Blockfrost. Is there any chance of something similar happening here if a new version isn't imposed on CIP-25 for this change?

I am also wondering @Ryun1 @KtorZ what token minting / management services we could query to ensure that metadata updates during burn isn't "a thing".

@rphair
Copy link
Collaborator

rphair commented Jul 11, 2023

@Crypto2099 no further response about the laissez-faire approach to declaring that burn transactions are not somehow constructively used to define token metadata... so @Ryun1 adding it for Review at next CIP meeting agenda # 70 (scheduled 18 July) so we can talk about it from a few different points of view before finally declaring that this just is a common-sense update.

@SmaugPool
Copy link
Contributor

SmaugPool commented Jul 11, 2023

For what it's worth, I can confirm that I always considered that "mint transactions" were excluding burn transactions and that pool.pm always ignored the later ones for CIP25 metadata (it considers only transactions with quantity > 0).

So I'm personally in favor of the additional clarification.

@rphair
Copy link
Collaborator

rphair commented Jul 11, 2023

thanks @SmaugPool - the second confirmation from your perspective is compelling. If @KtorZ @SebastienGllmt also don't know of an implementation that would require a version bump for CIP25 to make this change, then we might merge this upon review at the next meeting.

@rphair
Copy link
Collaborator

rphair commented Jul 18, 2023

@Ryun1 one topic that will come up at the CIP meeting today is whether you are intent on including this in your plan in the works to add versioning treatment explicitly to CIP-0001.

If this goes for a second meeting with nobody challenging the idea that burns should never update metadata, I think we should go ahead and merge it... but also think we should wait only if you want to include this question in your update plan... so if you have any reservations can you indicate them here? 🙏

@Ryun1
Copy link
Collaborator

Ryun1 commented Jul 20, 2023

@rphair

@Ryun1 one topic that will come up at the CIP meeting today is whether you are intent on including this in your plan in the works to add versioning treatment explicitly to CIP-0001.

So far I am not being as prescriptive for what to do with older CIPs, I am more focussed on giving direction for new CIPs.

Copy link
Collaborator

@Ryun1 Ryun1 left a comment

Choose a reason for hiding this comment

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

Although I am (maybe too) wary of altering active CIPs, this does seem to be a minor clarification with decent community consensus, on those grounds I approve.

  • as long as there are no community objections at the next CIP meeting (1st Aug 4pm UTC), we can place this in last check for the following meeting or merge.

@rphair
Copy link
Collaborator

rphair commented Jul 28, 2023

Promoting to Last Check here & on meeting agenda since @KtorZ and @SebastienGllmt have not really had a chance to blow the whistle on this yet... we can merge at meeting if no reservations there 😎

@rphair rphair added the State: Last Check Review favourable with disputes resolved; staged for merging. label Jul 28, 2023
@rphair rphair merged commit 89d32b8 into cardano-foundation:master Aug 1, 2023
mkazlauskas added a commit to input-output-hk/cardano-js-sdk that referenced this pull request Aug 7, 2023
mkazlauskas added a commit to input-output-hk/cardano-js-sdk that referenced this pull request Aug 9, 2023
mkazlauskas added a commit to input-output-hk/cardano-js-sdk that referenced this pull request Aug 17, 2023
AngelCastilloB pushed a commit to input-output-hk/cardano-js-sdk that referenced this pull request Aug 22, 2023
mkazlauskas added a commit to input-output-hk/cardano-js-sdk that referenced this pull request Sep 14, 2023
mkazlauskas added a commit to input-output-hk/cardano-js-sdk that referenced this pull request Sep 14, 2023
mkazlauskas added a commit to input-output-hk/cardano-js-sdk that referenced this pull request Sep 14, 2023
Ryun1 pushed a commit to Ryun1/CIPs that referenced this pull request Nov 17, 2023
@rphair rphair removed the State: Last Check Review favourable with disputes resolved; staged for merging. label Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Tokens Proposals belonging to the 'Tokens' category. Update Adds content or significantly reworks an existing proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants