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

[aptos-cli] Add Genesis tooling updates #1393

Merged
merged 1 commit into from
Jun 18, 2022

Conversation

gregnazario
Copy link
Contributor

Description

  • Add layout fields for future genesis features
  • Once these features are implemented, we'll tie them in

Test Plan

Genesis e2e tool test

@gregnazario gregnazario added the tooling For tooling related issues label Jun 14, 2022
#[serde(default)]
pub allow_new_validators: bool,
/// Initial lockup period for genesis validators
#[serde(default)]
Copy link
Contributor

@jjleng jjleng Jun 14, 2022

Choose a reason for hiding this comment

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

initial_lockup_period_duration_secs defaults to 0. Do these defaults make sense? What if people set allow_new_validators to be true and leave initial_lockup_period_duration_secs to its default?

Should we have better defaults for these three fields?

Copy link
Contributor

@movekevin movekevin Jun 14, 2022

Choose a reason for hiding this comment

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

+1. I think we also should have min lockup/max lockup and min/max stake set and reward rates in layout as well. These will be directly passed into the Stake module. See https://github.com/aptos-labs/aptos-core/blob/main/aptos-move/framework/aptos-framework/sources/configs/Stake.move#L33 and https://github.com/aptos-labs/aptos-core/blob/main/aptos-move/vm-genesis/src/lib.rs#L195

initial_lockup_default can then just be min_lockup

@sherry-x
Copy link
Contributor

After the discussion with Kevin and Zekun, we have couple things need to be configurable in genesis layout:

  • min stake to join validatorSet
  • min lockup time to join validatorSet
  • Default lockup expiration timestamp
  • allow_new_validators
  • Epoch length (1 hour / 1 day etc.)
  • We don't need the initial balance anymore since the original "stake amount" is sufficient for now
    See detailed discussion here https://aptos-org.slack.com/archives/C034W35ARFX/p1655334688236859

@gregnazario
Copy link
Contributor Author

@movekevin I will update this once my other two genesis PRs make it through since the location of the genesis tooling has moved

Copy link
Contributor

@jjleng jjleng left a comment

Choose a reason for hiding this comment

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

Overall, looks good to me. Will leave others to comment and approve.

crates/aptos-genesis/src/config.rs Outdated Show resolved Hide resolved
@@ -74,7 +77,7 @@ pub fn new_test_context(test_name: &'static str) -> TestContext {
ApiConfig::default(),
),
rng,
root_keys,
root_key,
Copy link
Contributor

Choose a reason for hiding this comment

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

shall we rename this key from root_key to mint_key? since the only thing it can do is really just mint

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

Comment on lines 36 to 50
pub allow_new_validators: bool,
/// Minimum stake to be in the validator set
pub min_stake: u64,
/// Minimum number of seconds to lockup staked coins
pub min_lockup_duration_secs: u64,
/// Duration of an epoch
pub epoch_duration_secs: u64,
/// Initial timestamp for genesis validators to be locked up
pub initial_lockup_timestamp: u64,
/// Min price per gas unit
pub min_price_per_gas_unit: u64,
Copy link
Contributor

Choose a reason for hiding this comment

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

We should align on those variable naming in https://github.com/aptos-labs/aptos-core/pull/1413/files to make them consistent

crates/aptos-genesis/src/config.rs Show resolved Hide resolved
crates/aptos-genesis/src/config.rs Show resolved Hide resolved
@gregnazario
Copy link
Contributor Author

/land

@aptos-bot
Copy link
Contributor

❗ Land has been canceled due to this PR being updated with new commits. Please issue another Land command if you want to requeue this PR.

@gregnazario
Copy link
Contributor Author

/land

@github-actions
Copy link
Contributor

Forge run: https://github.com/aptos-labs/aptos-core/actions/runs/2519290370
Forge test result: Forge test runner is terminated

@aptos-bot aptos-bot closed this in ed6c6d1 Jun 18, 2022
@aptos-bot aptos-bot merged commit ed6c6d1 into aptos-labs:main Jun 18, 2022
@leofisG
Copy link
Contributor

leofisG commented Jun 30, 2022

When will Node operator documentation be updated to reflect the added new field?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tooling For tooling related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants