Skip to content

Comments

chore: move CIP 29 & 30 to review #258

Merged
jcstein merged 3 commits intocelestiaorg:mainfrom
01builders:marko/bump_review
Mar 6, 2025
Merged

chore: move CIP 29 & 30 to review #258
jcstein merged 3 commits intocelestiaorg:mainfrom
01builders:marko/bump_review

Conversation

@tac0turtle
Copy link
Contributor

Overview

@tac0turtle tac0turtle marked this pull request as ready for review March 4, 2025 14:51
@rootulp
Copy link
Collaborator

rootulp commented Mar 4, 2025

markdown-lint failing

Copy link
Collaborator

@rootulp rootulp left a comment

Choose a reason for hiding this comment

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

CIP-30 looks ready to move to Review.

I found a few issues with CIP-29. LMK if you want me to address them. They seem worth addressing prior to moving to review.

| author | Dean Eigenmann ([@decanus](https://github.com/decanus)), Marko Baricevic ([@tac0turtle](https://github.com/tac0turtle)) |
| discussions-to | [Inflation Reduction Discussion Forum](https://forum.celestia.org/t/cip-reduce-inflation/1896) |
| status | Draft |
| status | Review |
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm re-reading CIP-29. I left this on the original PR but:

while other solutions have not been able to scale in a decentralized manner

seems incorrect and unwarranted. This claim isn't defended in the rest of the CIP so IMO it should be removed.

Remove the use of "we" in the spec section.

The spec section has a redundant line which can be removed

The only modification is to the base inflation parameter and the disinflation parameter; both drop by 33% on the next upgrade leading to lower yet sustainable inflation immediately

This section can be moved from spec to test cases:

Implementers MUST ensure:
The chain upgrade process includes the new inflation parameters without disrupting block production.
The new schedule is included in the next major version release to reflect the updated inflation rates on-chain in the next app version (v4).

This CIP is backwards incompatible and it must be included in a breaking release. This section needs to be updated.

No backward compatibility issues are anticipated.

The inflation parameters are applied in v4 so year 3 seems like a typo here:

Ensure that when the updated inflation parameters are applied at Year 3, the on-chain inflation rate reflects the new values instead of the old schedule.

The graph in the bottom is incorrect. The blue line does not reflect 50% reduced deflation because then the slope would be flatter than 33% deflation.

Copy link
Member

Choose a reason for hiding this comment

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

it looks like these are still outstanding cc @tac0turtle

This CIP is backwards incompatible and it must be included in a breaking release. This section needs to be updated.

No backward compatibility issues are anticipated.

The inflation parameters are applied in v4 so year 3 seems like a typo here:

Ensure that when the updated inflation parameters are applied at Year 3, the on-chain inflation rate reflects the new values instead of the old schedule.

The graph in the bottom is incorrect. The blue line does not reflect 50% reduced deflation because then the slope would be flatter than 33% deflation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The inflation parameters are applied in v4 so year 3 seems like a typo here:

Ensure that when the updated inflation parameters are applied at Year 3, the on-chain inflation rate reflects the new values instead of the old schedule.

This is a test case, i dont think it implies the change is at year 3. I can reword it though

tac0turtle and others added 2 commits March 4, 2025 17:19
Copy link
Member

@jcstein jcstein left a comment

Choose a reason for hiding this comment

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

LGTM

@jcstein jcstein requested a review from rootulp March 6, 2025 14:21
@jcstein jcstein merged commit 9485f0e into celestiaorg:main Mar 6, 2025
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants