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

CLI Tools #1226

Merged
merged 27 commits into from
May 13, 2020
Merged

CLI Tools #1226

merged 27 commits into from
May 13, 2020

Conversation

ajlopezrsk
Copy link
Contributor

Command line tools:

  • Export blocks
  • Export state
  • Show state info
  • Import blocks
  • Connect blocks
  • Execute blocks
  • Import State

@lgtm-com
Copy link

lgtm-com bot commented Apr 27, 2020

This pull request introduces 3 alerts when merging 8ca08a1 into 0df03b4 - view on LGTM.com

new alerts:

  • 3 for Potential input resource leak

@lgtm-com
Copy link

lgtm-com bot commented Apr 27, 2020

This pull request introduces 3 alerts when merging 0496d53 into 6c362f1 - view on LGTM.com

new alerts:

  • 3 for Potential input resource leak

@lgtm-com
Copy link

lgtm-com bot commented Apr 27, 2020

This pull request introduces 2 alerts when merging 79745a9 into 6c362f1 - view on LGTM.com

new alerts:

  • 2 for Potential input resource leak

@ajlopezrsk ajlopezrsk marked this pull request as ready for review April 27, 2020 20:23
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 7 Security Hotspots to review)
Code Smell A 6 Code Smells

67.8% 67.8% Coverage
0.0% 0.0% Duplication

Vovchyk
Vovchyk previously approved these changes May 8, 2020
BlockStore blockStore = ctx.getBlockStore();
ReceiptStore receiptStore = ctx.getReceiptStore();

String filename = args[0];
Copy link
Contributor

Choose a reason for hiding this comment

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

should we check the presence of the filename? same in the other classes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current code is oriented to be simple. We could improve the user experience if these tools are really used

Block block = blockFactory.decodeBlock(encoded);
block.seal();

blockchain.tryToConnect(block);
Copy link
Contributor

Choose a reason for hiding this comment

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

this returns a value that explains if the block has been connected or not. what happens if we don't check it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nothing. The supplied blocks could be connected best ones, or only connect, or even invalid. One of the use case of this tool is to run the external blocks, maybe to check the execution logs

@nicops
Copy link
Contributor

nicops commented May 8, 2020

I would add a println that says "this is an experimental/unsupported tool" in each of these main classes just to make clear that these tools are not meant for regular node use.

@ajlopezrsk
Copy link
Contributor Author

I would add a println that says "this is an experimental/unsupported tool" in each of these main classes just to make clear that these tools are not meant for regular node use.

Well, I would prefer not pollute output with message, keep the first implementation simple, so we could improve them when we have more use pattern info

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 7 Security Hotspots to review)
Code Smell A 6 Code Smells

67.8% 67.8% Coverage
0.0% 0.0% Duplication

@ajlopezrsk
Copy link
Contributor Author

pipeline:run

@ajlopezrsk
Copy link
Contributor Author

pipeline: run

1 similar comment
@ajlopezrsk
Copy link
Contributor Author

pipeline: run

@patogallaiovlabs
Copy link
Contributor

pipeline:run

1 similar comment
@patogallaiovlabs
Copy link
Contributor

pipeline:run

@ajlopezrsk ajlopezrsk merged commit c441da8 into master May 13, 2020
@delete-merged-branch delete-merged-branch bot deleted the clitools branch May 13, 2020 18:22
@aeidelman aeidelman added this to the Papyrus v2.1.0 milestone Jun 9, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants