-
Notifications
You must be signed in to change notification settings - Fork 58
docs: update dip-0023 - removed mentions of non existing min_activation_height #153
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
Conversation
thephez
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm waiting for @PastaPastaPasta to comment. Perhaps it should be discussed on a Core call?
PastaPastaPasta
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generally LGTM; @thephez please review language
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThe status of DIP 23 was updated to "Final" in the proposal summary table. The DIP 23 document was revised to clarify Stage 0 and Stage 2 activation descriptions by removing specific formulas and parameters, emphasizing that miner signalling solely determines activation progression after the initial cycle. Changes
Estimated code review effort2 (10–30 minutes) Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
dip-0023.md (2)
50-54: Clarify wording in Stage 0“Impossible to move from
defined” is slightly ambiguous. Consider rephrasing to “The deployment state remainsdefined; no transition is possible during Stage 0.” – this preserves the intent while improving readability.
87-89: Minor grammar tweak“Once a new cycle started…” → “Once a new cycle starts…”.
Small polish that avoids tense mismatch.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(1 hunks)dip-0023.md(2 hunks)
🔇 Additional comments (1)
dip-0023.md (1)
78-81: Variable naming still implies implicit calculation – double-check intentYou removed
min_activation_height, yet keep a formula that setsmasternode_activation_heightbased on the block wheremnhfsignalis mined. That’s fine, but ensure older references tomin_activation_heightwere purged elsewhere in the doc and in implementation notes so no stale parameter survives.
thephez
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few rewording suggestions. We should also update the status in the DIP as indicated by the coderabbit comment.
Co-authored-by: thephez <thephez@users.noreply.github.com>
see changes.
This PR replaces https://github.com/dashpay/dips/pull/146/files
Summary by CodeRabbit