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

Initial review of Moonbeam (Phase 0) #924

Merged
merged 29 commits into from
Oct 29, 2021
Merged

Conversation

crystalin
Copy link
Collaborator

No description provided.

@crystalin crystalin added A0-pleasereview Pull request needs code review. B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes labels Oct 25, 2021
Comment on lines 89 to 90
pub const MOONRIVER_FACTOR: Balance = 100; // 100 more total supply

Copy link
Contributor

Choose a reason for hiding this comment

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

Since Moonbeam is the flagship product, does it make more sense to have such a factor in the Moonriver runtime?

Copy link
Contributor

Choose a reason for hiding this comment

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

It does, but this would imply division (or multiplying by a fraction) :)

I agree, though, it makes more sense to define this either (1) as a portion of total/genesis supply or (2) relative to GMLR

/// Default BlocksPerRound is every hour (300 * 12 second block times)
pub const DefaultBlocksPerRound: u32 = 300;
/// Collator candidate exits are delayed by 2 hours (2 * 300 * block_time)
/// Default BlocksPerRound is every 4 hours (1200 * 12 second block times)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Default BlocksPerRound is every 4 hours (1200 * 12 second block times)
/// Default BlocksPerRound is every 4 hours

runtime/moonbeam/src/lib.rs Outdated Show resolved Hide resolved
runtime/moonbeam/src/lib.rs Show resolved Hide resolved
Copy link
Contributor

@tgmichel tgmichel left a comment

Choose a reason for hiding this comment

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

lgtm

runtime/moonbeam/src/lib.rs Outdated Show resolved Hide resolved
runtime/moonbeam/src/lib.rs Outdated Show resolved Hide resolved
@crystalin crystalin changed the title Initial review of Moonbeam Initial review of Moonbeam (Phase 0) Oct 26, 2021
Copy link
Contributor

@notlesh notlesh left a comment

Choose a reason for hiding this comment

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

🚀 🌔

crystalin and others added 2 commits October 28, 2021 18:04
Co-authored-by: Joshy Orndorff <JoshOrndorff@users.noreply.github.com>
@crystalin crystalin merged commit 3789950 into master Oct 29, 2021
@crystalin crystalin deleted the crystalin-moonbeam-setup branch October 29, 2021 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A0-pleasereview Pull request needs code review. B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants