TrueBitFoundation / truebit-os

[DEPRECATED] Interactive client
Apache License 2.0
82 stars 26 forks source link

Add simple vorpal example #21

Closed OR13 closed 6 years ago

OR13 commented 6 years ago

If this looks good, I can proceed to replicate all existing shell commands. vorpal can help enforce required arguments, but the existing shell seemed to use optional arguments as required. I defaulted them to make them optional, but they could also be made required, and no additional error handling logic would be needed.

https://github.com/dthree/vorpal/wiki/api-%7C-vorpal.command#vorpalcommandcommand-description

Since this tool is initially for testing, I recommend not making arguments required and picking reasonable defaults so the flow can be quickly run through, like so:

› npm run truebit

> truebit-client@1.0.0 truebit /Users/orie/Desktop/tech-spikes/truebit-os
> node ./cli/index.js basic-client/config.json

  _____                         _       _   _       
 |_   _|  _ __   _   _    ___  | |__   (_) | |_   _ 
   | |   | '__| | | | |  / _ \ | '_ \  | | | __| (_)
   | |   | |    | |_| | |  __/ | |_) | | | | |_   _ 
   |_|   |_|     \__,_|  \___| |_.__/  |_|  \__| (_)

  _            _                _                            _  __       
 | |_ __ _ ___| | __  ___  ___ | |_   _____  __   _____ _ __(_)/ _|_   _ 
 | __/ _` / __| |/ / / __|/ _ \| \ \ / / _ \ \ \ / / _ \ '__| | |_| | | |
 | || (_| \__ \   <  \__ \ (_) | |\ V /  __/  \ V /  __/ |  | |  _| |_| |
  \__\__,_|___/_|\_\ |___/\___/|_| \_/ \___|   \_/ \___|_|  |_|_|  \__, |
                                                                   |___/ 
🎲   $ 2018-06-04 22:05:21 [console] info: Truebit OS has been initialized with config at basic-client/config.json
🎲   $ start task
2018-06-04 22:05:26 [console] info: Task Giver initialized
🎲   $ start solve
2018-06-04 22:05:29 [console] info: Solver initialized
🎲   $ task
🎲   $ 2018-06-04 22:05:34 [console] info: Task submitted 0xbfdf4d4185cf59347a6014a6054b83767a901d14fd6b70e4ebbbeaec6bfbbe75
🎲   $ skip
🎲   $ Error: VM Exception while processing transaction: revert
OR13 commented 6 years ago

I recommend not reusing words such as task for commands / agents. perhapse:

start task-submitter -a 0
submit-task -a 0 -t testTask.json
hswick commented 6 years ago

The vorpal stuff seems very reasonable.

In terms of commands, I agree that the task stuff seems confusing. I think we should do this:

start task -a 0
submit-task -a 0 -t testTask.json
OR13 commented 6 years ago

Cool, I will continue, and use that approach.

OR13 commented 6 years ago

Leaving this here for reference: https://github.com/dthree/cash/tree/master/test

Testing CLI's is difficult. The functions can be tested, or the full cli can be tested.

I think the best approach is to have vorpal call a library function, and then write tests for each of those functions. This saves you from needing to handle stdout/stderr.

hswick commented 6 years ago

Testing stdout would be fancy, but probably not necessary for the time. The functions that the cli's should be calling are mostly in the tests now, so I think that will be sufficient.

In terms of the UI, manually going through to check isn't too bad since there aren't too many commands at the moment.

OR13 commented 6 years ago

I've taken some stylistic liberties here, happy to remove them (the dice). Just want to show what can be done. I've also held off on changing any of the naming until we are happy with behavior. Probably best not to mix that with this PR.

Regarding testing, I've added tests for the cliLIb which provides all the functions used by the cli.

This allows for unit testing of the cli behavior without the need to capture shell output.

From an organization standpoint it seems like this project should have unit tests around the cliLib, and flows.

For example, it is possible to use the cliLib to test flows of commands, for example cases where you do not initialize a solver, but call submitTask.

OR13 commented 6 years ago

travis is so picky... this passed on my side. https://travis-ci.org/OR13/truebit-os/builds/389775141

I need to add the tests that I have written and test again.

OR13 commented 6 years ago

Objective here is to get the cliLib tests to pass along side the os tests. I'm seeing some revert failure when running os-challenge.

This is caused by the addition of the verifier. The os-challenge test does not actually test the verifier, and its presence causes failure.

I'm happy to tackle fixing that test in a separate pr.

OR13 commented 6 years ago

If we are happy with travis tests passing in this form:

https://travis-ci.org/TrueBitFoundation/truebit-os/builds/390175190#L914

I suggest we merge this PR, and I can tackle the verifier (os-challenge) bit next.

I can also remove reference to the old shell and cleanup a bit, but I don't want to do that unless its requested.

hswick commented 6 years ago

Great work! Yes please remove any references of old shell, we have no use for it anymore.

OR13 commented 6 years ago

Now we know why the builds were failing... If i had a penny for every-time truffle / testrpc / ganache / web3 version slipped and broke my stuff... I'd have lots of pennies....

https://travis-ci.org/TrueBitFoundation/truebit-os/jobs/391048018#L461

OR13 commented 6 years ago

I'd say this is safe to merge now.

hswick commented 6 years ago

Looks good! Really appreciate the effort on this!