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

add quit command to cli_wallet #1050 #1104

Merged
merged 18 commits into from
Jul 27, 2018

Conversation

nanomobile
Copy link
Contributor

@nanomobile nanomobile commented Jul 1, 2018

PR for #1050.

libraries/wallet/wallet.cpp Outdated Show resolved Hide resolved
tests/cli/main.cpp Outdated Show resolved Hide resolved
@nanomobile nanomobile force-pushed the valera_issue_1050 branch 5 times, most recently from da4b84e to 723ee56 Compare July 1, 2018 15:20
libraries/wallet/wallet.cpp Outdated Show resolved Hide resolved
libraries/wallet/include/graphene/wallet/wallet.hpp Outdated Show resolved Hide resolved
Copy link
Contributor Author

@nanomobile nanomobile left a comment

Choose a reason for hiding this comment

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

just FC_THROW isn't enough as I've tested it just print message and that's all

libraries/wallet/wallet.cpp Outdated Show resolved Hide resolved
@nanomobile nanomobile force-pushed the valera_issue_1050 branch from 723ee56 to 6d9ba1c Compare July 1, 2018 15:38
@nanomobile nanomobile force-pushed the valera_issue_1050 branch 2 times, most recently from 9a22871 to c61a994 Compare July 2, 2018 09:24
@nanomobile
Copy link
Contributor Author

nanomobile commented Jul 2, 2018

@ryanRfox @abitmore @oxarbitrage @jmjatlanta @clockworkgr I've added test case as requested and try/catch block, still cannot understand what to use instead of EXIT, can anybody advise anything what is the best to use instead of EXIT ?

c61a994

Thanks everybody !

@oxarbitrage
Copy link
Member

please research for an alternative and present for discussion all the options you can find, we don't know either.

@oxarbitrage
Copy link
Member

the test is not enough in my opinion. it needs to create some accounts, move some balances, then quit. then open again, use the file to restore and check if the movements done are there.

@nanomobile
Copy link
Contributor Author

ok sure ! Thanks ! 👍

@nanomobile
Copy link
Contributor Author

I see that even in libraries exit is still used in many different places, for example:

  1. https://github.com/bitshares/bitshares-core/blob/master/libraries/app/application.cpp#L425
  2. https://github.com/bitshares/bitshares-core/blob/master/libraries/app/application.cpp#L988
  3. https://github.com/bitshares/bitshares-core/blob/master/libraries/app/application.cpp#L1019

and in the whole project there are also other places with exit method. So what can you say about that guys @oxarbitrage @abitmore @ryanRfox @jmjatlanta @clockworkgr ? Why there is it OK but not here ? Or there isn't OK too ?

@abitmore
Copy link
Member

abitmore commented Jul 2, 2018

@nanomobile thanks for the report. We need to fix those. Exiting from a library makes it very hard if not impossible to test with boost testing framework. For the 2 exiting on error, should change to throw an exception. For the other one, probably should return a special value then check at where calling it. Need to create new issues for these though.

@nanomobile
Copy link
Contributor Author

there are not only 3 places with exit method, we need to fix all IMHO

@abitmore
Copy link
Member

abitmore commented Jul 2, 2018

Created new issue: #1110, please feel free to add more findings there. Thanks.

@cogutvalera
Copy link
Member

just rebased current changes in this PR to be synced with core/develop, soon will make more commits in order to make everything correctly and close this issue

@cogutvalera
Copy link
Member

instead of using exit(0) it's much better to use _Exit(0) as described here http://www.cplusplus.com/reference/cstdlib/_Exit/ :

Terminate calling process
Terminates the process normally by returning control to the host environment, but without performing any of the regular cleanup tasks for terminating processes (as function exit does).

No object destructors, nor functions registered by atexit or at_quick_exit are called.

Whether C streams are closed and/or flushed, and files open with tmpfile are removed depends on the particular system or library implementation.

If status is zero or EXIT_SUCCESS, a successful termination status is returned to the host environment.
If status is EXIT_FAILURE, an unsuccessful termination status is returned to the host environment.
Otherwise, the status returned depends on the system and library implementation.
Data races
Concurrently calling this function multiple times has no effect.

Exceptions (C++)
No-throw guarantee: this function never throws exceptions.

@cogutvalera
Copy link
Member

guys check and test this pull request please if this issue can be closed or not yet ...
Thanks !

@cogutvalera
Copy link
Member

ok sure ! Thanks ! Will fix now !

@pmconrad
Copy link
Contributor

Now it's wrong. void quit() doesn't return anything.
If you want to keep the comment you must fix the perl script. :-)

@cogutvalera
Copy link
Member

ok understood :-) will remove it :-)

@abitmore
Copy link
Member

@cogutvalera please bump FC to latest version to include the time_point_sec stringify test fix.

@cogutvalera
Copy link
Member

Bump FC is done ! Thank you guys !

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.

How to validate the wallet file:

tests/cli/main.cpp Show resolved Hide resolved
@abitmore
Copy link
Member

Manual test:

new >>> quit
quit
2551611ms th_a       wallet.cpp:780                quit                 ] Quitting Cli Wallet ...
9 canceled_exception: Canceled

2551611ms th_a       wallet.cpp:799                save_wallet_file     ] saving wallet to file wallet.json
pure virtual method called
terminate called without an active exception
Aborted (core dumped)
  • Best if 9 canceled_exception: Canceled is not there, but this is a minor issue.
  • wallet file does get saved
  • the crash after saved wallet file is another common issue, if press Ctrl+D:
new >>>

2614126ms th_a       wallet.cpp:799                save_wallet_file     ] saving wallet to file wallet.json
pure virtual method called
terminate called without an active exception
Aborted (core dumped)

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.

6 participants