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.

Sign up for GitHub

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

Jump to bottom

Create command line arguments to set genesis staking keys, VRF keys, and pool Id #1092

Merged
merged 26 commits into from
Aug 4, 2023
Merged

Create command line arguments to set genesis staking keys, VRF keys, and pool Id #1092

merged 26 commits into from
Aug 4, 2023

Conversation

alfiedotwtf
Copy link
Contributor

@alfiedotwtf alfiedotwtf commented Jul 28, 2023 •
edited

This is the last item for #1004

@alfiedotwtf alfiedotwtf mentioned this pull request Jul 28, 2023
Closed
5 tasks
iljakuklic
iljakuklic reviewed Jul 28, 2023
View reviewed changes
Copy link
Contributor

@iljakuklic iljakuklic left a comment

Choose a reason for hiding this comment

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

A couple of initial observations:

common/src/chain/config/mod.rs Outdated Show resolved Hide resolved
node-lib/src/regtest_options.rs Outdated Show resolved Hide resolved
test/functional/blockprod_generate_pos_blocks_rand_genesis_keys.py Outdated Show resolved Hide resolved
common/src/chain/config/mod.rs Outdated Show resolved Hide resolved
@alfiedotwtf alfiedotwtf force-pushed the alfie/genesis-staking-settings branch from c3cf9bb to c696de7 Compare July 31, 2023 10:49
iljakuklic
iljakuklic reviewed Jul 31, 2023
View reviewed changes
Copy link
Contributor

@iljakuklic iljakuklic left a comment

Choose a reason for hiding this comment

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

Some more comments about input parsing below

common/src/chain/config/mod.rs Outdated Show resolved Hide resolved
common/src/chain/config/mod.rs Outdated Show resolved Hide resolved
node-lib/src/regtest_options.rs Outdated Show resolved Hide resolved
common/src/chain/config/mod.rs Outdated Show resolved Hide resolved
common/src/chain/config/mod.rs Outdated Show resolved Hide resolved
common/src/chain/config/mod.rs Outdated Show resolved Hide resolved
azarovh
azarovh reviewed Aug 1, 2023
View reviewed changes
Copy link
Member

@azarovh azarovh left a comment

Choose a reason for hiding this comment

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

Agree with Lukas that it makes sense to make GenesisStakingSettings a proper struct with fields and parsing and decoding implemented in the constructor.

node-lib/src/runner.rs Outdated Show resolved Hide resolved
@alfiedotwtf alfiedotwtf force-pushed the alfie/genesis-staking-settings branch from 546e024 to 78a02c3 Compare August 1, 2023 08:12
iljakuklic
iljakuklic reviewed Aug 1, 2023
View reviewed changes
common/src/chain/config/regtest.rs Outdated Show resolved Hide resolved
common/src/chain/config/regtest.rs Outdated Show resolved Hide resolved
@alfiedotwtf alfiedotwtf marked this pull request as draft August 1, 2023 08:57
@TheQuantumPhysicist
Copy link
Collaborator

This is looking fine. I'll review again after addressing the comments.

@alfiedotwtf alfiedotwtf force-pushed the alfie/genesis-staking-settings branch from 78a02c3 to d52936a Compare August 2, 2023 08:08
@alfiedotwtf alfiedotwtf marked this pull request as ready for review August 2, 2023 08:25
iljakuklic
iljakuklic approved these changes Aug 2, 2023
View reviewed changes
Copy link
Contributor

@iljakuklic iljakuklic left a comment

Choose a reason for hiding this comment

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

Looks good. A couple more minor suggestions here:

common/src/chain/config/regtest.rs Outdated Show resolved Hide resolved
common/src/chain/config/regtest.rs Show resolved Hide resolved
test/functional/blockprod_generate_pos_blocks_rand_genesis_keys.py Outdated Show resolved Hide resolved
azarovh
azarovh approved these changes Aug 2, 2023
View reviewed changes
common/src/chain/config/regtest.rs Outdated Show resolved Hide resolved
TheQuantumPhysicist
TheQuantumPhysicist approved these changes Aug 2, 2023
View reviewed changes
Copy link
Collaborator

@TheQuantumPhysicist TheQuantumPhysicist left a comment

Choose a reason for hiding this comment

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

Besides the comments, I think this is looking shiny enough.

@alfiedotwtf alfiedotwtf mentioned this pull request Aug 3, 2023
Closed
@alfiedotwtf alfiedotwtf force-pushed the alfie/genesis-staking-settings branch from d52936a to 139bd89 Compare August 3, 2023 08:27
iljakuklic
iljakuklic approved these changes Aug 3, 2023
View reviewed changes
Cargo.toml Outdated Show resolved Hide resolved
TheQuantumPhysicist
TheQuantumPhysicist reviewed Aug 3, 2023
View reviewed changes
test/functional/blockprod_generate_pos_blocks.py Outdated
@@ -25,6 +25,8 @@

import random, time

COIN = 100_000_000_000
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be a global definition somewhere. I bet there's already a definition somewhere for bitcoin. We shouldn't redefine it for every test.

@alfiedotwtf
Allow test framework to not exit on success
9091f81
@alfiedotwtf
Add Genesis staking settings parsing
b869720
@alfiedotwtf
Add tets for genesis staking settings
2d56a2f
@alfiedotwtf
Remove regex for command-line parsing
67e4dbe
@alfiedotwtf
Add a type for genesis staking settings to be used by Clap
1cff1dd
@alfiedotwtf
Adding further explaination for the two main() calls
fd3a688
@alfiedotwtf
Fix do_checks
a1bfb3f
@alfiedotwtf
Moved RegTest-specific code into its own file
c7b51c9
@alfiedotwtf
Lifted more handling of genesis settings to the type
1d0e413
@alfiedotwtf
Better Genesis staking settings error handling
5c992dc
@alfiedotwtf
Make default values idiomatic
4dd0e6b
@alfiedotwtf
Fix broken round tripping of default clap custom variables
068452c
@alfiedotwtf
Remove "genesis_" prefix and more default() fixes
9af4cd4
@alfiedotwtf
Moving into constants
f83b607
@alfiedotwtf
Fix typo
84175e0
@alfiedotwtf
Adding unit tests for GenesisStakingSettings
2ca3b32
@alfiedotwtf
Fix panicking decoding errors
076562f
@alfiedotwtf
Fix for rebase
f7f7cc6
@alfiedotwtf
Make large coin values readable
12b089b
@alfiedotwtf
Don't return a clone
72c85ff
@alfiedotwtf
Error when staking settings are missing a value
d24809e
@alfiedotwtf
Error when settings are not unique
7597247
@alfiedotwtf
Removing regex dependency
4e74444
@alfiedotwtf
Gate regtest configuration options to only the regtest network
65bfe3a
@alfiedotwtf
Factor out reusable names
5e548d8
@alfiedotwtf alfiedotwtf force-pushed the alfie/genesis-staking-settings branch from 5178993 to 5e548d8 Compare August 4, 2023 06:33
@alfiedotwtf alfiedotwtf merged commit 4d401c7 into master Aug 4, 2023
23 checks passed
@alfiedotwtf alfiedotwtf deleted the alfie/genesis-staking-settings branch August 4, 2023 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TheQuantumPhysicist TheQuantumPhysicist TheQuantumPhysicist approved these changes

@iljakuklic iljakuklic iljakuklic approved these changes

@azarovh azarovh azarovh approved these changes

@muursh muursh Awaiting requested review from muursh

@erubboli erubboli Awaiting requested review from erubboli erubboli is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@alfiedotwtf @TheQuantumPhysicist @iljakuklic @azarovh