Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Issue1011, Add CLI startup option to only generate keys #1039

Merged
merged 2 commits into from
Jun 18, 2018
Merged

Issue1011, Add CLI startup option to only generate keys #1039

merged 2 commits into from
Jun 18, 2018

Conversation

windycrypto
Copy link
Member

fix #1011

For code reuse, i moved origin wallet_api::suggest_brain_key's code into graphene::wallet::utility to make it static method.

self test ok.

@ryanRfox ryanRfox added 1c Task Task for team member to perform. Description may contain a Task List and reference child Sub-Tasks 2g Final Review Status indicating the solution passes the test case(s) and is being reviewed for final acceptance. 3c Enhancement Classification indicating a change to the functionality of the existing imlementation 6 CLI Impact flag identifying the command line interface (CLI) wallet application labels Jun 11, 2018
Copy link
Contributor

@jmjatlanta jmjatlanta 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. Thanks!

@@ -86,6 +86,7 @@ int main( int argc, char** argv )
("daemon,d", "Run the wallet in daemon mode" )
("wallet-file,w", bpo::value<string>()->implicit_value("wallet.json"), "wallet to load")
("chain-id", bpo::value<string>(), "chain ID to connect to")
("suggest-brain-key", "Suggest a safe brain key to use for craeting your account")
Copy link
Contributor

@pmconrad pmconrad Jun 11, 2018

Choose a reason for hiding this comment

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

Typo: craeting

Copy link
Member Author

Choose a reason for hiding this comment

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

ah oh.. thanks~ fixed

@ryanRfox
Copy link
Contributor

ryanRfox commented Jun 13, 2018

Prior to merge, may I request of @cifer-lee on #1011:

  • Write test case
  • Add test case to test suite
  • Update documentation (@cedar-book can assist with this)

@ryanRfox ryanRfox added the 5a Docs Needed Status specific to Documentation indicating the need for proper documentation to be added label Jun 13, 2018
@abitmore
Copy link
Member

@ryanRfox although having test cases for everything would be a nice thing, it comes with costs. IMHO we can skip test case here, and put efforts elsewhere, for better use.

@jmjatlanta
Copy link
Contributor

@ryanRfox I'd like to get this merged, but I don't want to forget about the documentation part. What is the best way to make sure that it gets documented? Merge and close this PR, but keep the issue opened?

@abitmore
Copy link
Member

@jmjatlanta create a new issue for documentation.

@abitmore
Copy link
Member

I think we can also move the request for documentation to the issue but not leave it here.

@jmjatlanta jmjatlanta merged commit 83af94e into bitshares:develop Jun 18, 2018
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
1c Task Task for team member to perform. Description may contain a Task List and reference child Sub-Tasks 2g Final Review Status indicating the solution passes the test case(s) and is being reviewed for final acceptance. 3c Enhancement Classification indicating a change to the functionality of the existing imlementation 5a Docs Needed Status specific to Documentation indicating the need for proper documentation to be added 6 CLI Impact flag identifying the command line interface (CLI) wallet application
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants