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

cmd/createcluster: add fee-recipient and name flags #878

Merged
merged 2 commits into from
Jul 29, 2022

Conversation

dB2510
Copy link
Contributor

@dB2510 dB2510 commented Jul 29, 2022

Adds fee recipient and name flags and use withdrawal address, fee recipient address and name in cluster lock creation. This will be useful for those who want to create a cluster without going through dkg process preferably centralised stakers.

category: refactor
ticket: #879

…pient address and name in cluster lock creation
@dB2510 dB2510 changed the title cmd/createcluster: add fee-recipient and name flag cmd/createcluster: add fee-recipient and name flags Jul 29, 2022
@codecov
Copy link

codecov bot commented Jul 29, 2022

Codecov Report

Merging #878 (554c0f5) into main (02d7965) will decrease coverage by 0.01%.
The diff coverage is 50.00%.

❗ Current head 554c0f5 differs from pull request most recent head c058c03. Consider uploading reports for the commit c058c03 to get more accurate results

@@            Coverage Diff             @@
##             main     #878      +/-   ##
==========================================
- Coverage   54.43%   54.41%   -0.02%     
==========================================
  Files         114      114              
  Lines       12231    12229       -2     
==========================================
- Hits         6658     6655       -3     
- Misses       4613     4614       +1     
  Partials      960      960              
Impacted Files Coverage Δ
cmd/createcluster.go 48.28% <50.00%> (-1.15%) ⬇️
app/app.go 56.71% <0.00%> (-1.63%) ⬇️
core/qbft/qbft.go 81.97% <0.00%> (+0.42%) ⬆️
core/dutydb/memory.go 71.91% <0.00%> (+1.27%) ⬆️
cmd/createdkg.go 76.74% <0.00%> (+3.48%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 02d7965...c058c03. Read the comment docs.

@@ -81,9 +83,11 @@ func newCreateClusterCmd(runFunc func(io.Writer, clusterConfig) error) *cobra.Co
}

func bindClusterFlags(flags *pflag.FlagSet, config *clusterConfig) {
flags.StringVar(&config.Name, "name", clusterName, "Optional cosmetic cluster name")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest not defaulting to "", suggest using current default that is clusterName=local.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, technically not optional if there is a default, suggest removing "Optional" from description.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or if empty cluster name is fine, then at least remove clusterName const

@@ -352,7 +356,7 @@ func newLock(conf clusterConfig, dvs []tbls.TSS, peers []p2p.Peer) (cluster.Lock
})
}

def := cluster.NewDefinition(clusterName, len(dvs), conf.Threshold, "", "", "", ops, rand.Reader)
def := cluster.NewDefinition(conf.Name, len(dvs), conf.Threshold, conf.FeeRecipient, conf.WithdrawalAddr, networkToForkVersion[conf.Network], ops, rand.Reader)
Copy link
Contributor

Choose a reason for hiding this comment

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

split very long lines

@dB2510 dB2510 added the merge when ready Indicates bulldozer bot may merge when all checks pass label Jul 29, 2022
@dB2510 dB2510 linked an issue Jul 29, 2022 that may be closed by this pull request
@obol-bulldozer obol-bulldozer bot merged commit 20835a5 into main Jul 29, 2022
@obol-bulldozer obol-bulldozer bot deleted the dhruv/createcluster branch July 29, 2022 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge when ready Indicates bulldozer bot may merge when all checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add --num-validators, --name and --fee-recipient flags to create cluster
2 participants