Skip to content

Conversation

@edmundnoble
Copy link
Contributor

Change-Id: Id0000000e1649a5b99329990bf39c9395e7cac69

Change-Id: Id0000000e1649a5b99329990bf39c9395e7cac69
Copy link
Contributor

@larskuhtz larskuhtz left a comment

Choose a reason for hiding this comment

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

Approved with one more general question about the command line parser for ChainwebVersion.

<*> optional (textOption @Fork (long "fork-upper-bound" <> help "(development mode only) the latest fork the node will enable"))
<*> optional (BlockDelay <$> textOption (long "block-delay" <> help "(development mode only) the block delay in seconds per block"))
<*> switch (long "disable-pow" <> help "(development mode only) disable proof of work check")
<*> optional (switch (long "disable-pow" <> help "(development mode only) disable proof of work check"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why parseVersion is not following the idiom for command lines parers from configuration-tools that is used everywhere else in Chainweb node? That idiom would prevent bugs like this one.

I guess, and configuration of ChainwebVersion is special and doesn't fit that pattern? Or is it just an oversight?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes the weirdness here is that we parse the chainweb version, and then we parse a bunch of options that also alter this chainweb version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants