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.

Sign up for GitHub

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

Jump to bottom

Add PCLI command to sign account balance files #6264

Merged
merged 19 commits into from Apr 28, 2023
Merged

Add PCLI command to sign account balance files #6264

merged 19 commits into from Apr 28, 2023

Conversation

iwsimon
Copy link
Contributor

@iwsimon iwsimon commented Apr 26, 2023 •
edited

Description:

Related issue(s):

Fixes #5820

To run the test, use the command:

pcli account-balance sign keyFile keyFilePassword keyAlias -d=signedFiles -p=toBeSignedFile

Notes for reviewer:

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

@iwsimon iwsimon requested review from a team as code owners April 26, 2023 19:14
@iwsimon iwsimon self-assigned this Apr 26, 2023
@iwsimon iwsimon added this to the v0.38 milestone Apr 26, 2023
povolev15
povolev15 approved these changes Apr 26, 2023
View reviewed changes
settings.gradle.kts Outdated Show resolved Hide resolved
povolev15
povolev15 previously approved these changes Apr 26, 2023
View reviewed changes
settings.gradle.kts Outdated Show resolved Hide resolved
@iwsimon
added Sign-off
010644e 
Signed-off-by: Iris Simon <iris.simon@swirldslabs.com>
@github-actions
Copy link

github-actions bot commented Apr 26, 2023 •
edited

Node: Unit Test Results

  1 335 files    1 335 suites   1h 13m 0s ⏱️
97 387 tests 97 379 ✔️ 8 💤 0 ❌
99 019 runs  99 011 ✔️ 8 💤 0 ❌

Results for commit ef5ad4e.

♻️ This comment has been updated with latest results.

@iwsimon iwsimon requested a review from alittley April 26, 2023 19:38
@github-actions
Copy link

github-actions bot commented Apr 26, 2023 •
edited

Node: Integration Test Results

    3 files      3 suites   15m 14s ⏱️
151 tests 151 ✔️ 0 💤 0 ❌
152 runs  152 ✔️ 0 💤 0 ❌

Results for commit ef5ad4e.

♻️ This comment has been updated with latest results.

@iwsimon
Revert "added Sign-off"
5f30b76 
This reverts commit 010644e.
david-bakin-sl
david-bakin-sl reviewed Apr 26, 2023
View reviewed changes
Copy link
Member

@david-bakin-sl david-bakin-sl left a comment

Choose a reason for hiding this comment

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

Some consistency between System.{out|err} vs logging vs throwing an exception never handled would be an improvement (especially as this tool will be used in a production environment by professionals (who aren't developers).

Otherwise just minor comments.

settings.gradle.kts Outdated Show resolved Hide resolved
hedera-node/cli-clients/src/main/java/com/hedera/services/cli/sign/SignUtils.java Outdated Show resolved Hide resolved
hedera-node/cli-clients/src/main/java/com/hedera/services/cli/sign/SignUtils.java Outdated Show resolved Hide resolved
hedera-node/cli-clients/src/main/java/com/hedera/services/cli/sign/SignUtils.java Show resolved Hide resolved
hedera-node/cli-clients/src/main/java/com/hedera/services/cli/sign/AccountBalanceType.java Outdated Show resolved Hide resolved
...-node/cli-clients/src/main/java/com/hedera/services/cli/sign/AccountBalanceSigningUtils.java Show resolved Hide resolved
...-node/cli-clients/src/main/java/com/hedera/services/cli/sign/AccountBalanceSigningUtils.java Show resolved Hide resolved
...-node/cli-clients/src/main/java/com/hedera/services/cli/sign/AccountBalanceSigningUtils.java Show resolved Hide resolved
hedera-node/cli-clients/src/main/java/com/hedera/services/cli/sign/SignUtils.java Show resolved Hide resolved
...-node/cli-clients/src/main/java/com/hedera/services/cli/sign/AccountBalanceSigningUtils.java Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Apr 26, 2023 •
edited

Node: E2E Test Results

    1 files      1 suites   17m 7s ⏱️
309 tests 309 ✔️ 0 💤 0 ❌
327 runs  327 ✔️ 0 💤 0 ❌

Results for commit ef5ad4e.

♻️ This comment has been updated with latest results.

iwsimon and others added 2 commits April 26, 2023 16:56
@iwsimon
Merge branch 'develop' into 5820-sign-services
c648c62
@iwsimon
updated with reviews.
34d0ecd 
Signed-off-by: Iris Simon <iris.simon@swirldslabs.com>
david-bakin-sl
david-bakin-sl previously approved these changes Apr 28, 2023
View reviewed changes
Copy link
Member

@david-bakin-sl david-bakin-sl left a comment

Choose a reason for hiding this comment

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

The is a command line tool. For production use yes. But a CLI, not a server we're depending on. I think it is ok for it to be rough around the edges and get cleaned up in a very closely-following PR. Am I alone in this?

povolev15
povolev15 previously approved these changes Apr 28, 2023
View reviewed changes
@povolev15
Copy link
Contributor

The is a command line tool. For production use yes. But a CLI, not a server we're depending on. I think it is ok for it to be rough around the edges and get cleaned up in a very closely-following PR. Am I alone in this?

I agree with you but this tool was decided by Nathan as production on the channel and we need to merge it today, it is an endless story let's try to focus on merging it

@povolev15 @qnswirlds
Update hedera-node/cli-clients/src/main/java/module-info.java
ef5ad4e 
remove the logging imports

Co-authored-by: Quan Nguyen <quan@swirldslabs.com>
Signed-off-by: Lev Povolotsky <16233475+povolev15@users.noreply.github.com>
@povolev15 povolev15 dismissed stale reviews from david-bakin-sl and themself via ef5ad4e April 28, 2023 13:28
@nickpoorman
Copy link
Contributor

@david-bakin-sl, what are the things that are rough around the edges that you would like fixed? We should get those done today before merging this in.

@nickpoorman
Copy link
Contributor

@iwsimon and @povolev15, this also needs tests before it can be merged.

@david-bakin-sl
Copy link
Member

@david-bakin-sl, what are the things that are rough around the edges that you would like fixed? We should get those done today before merging this in.

As far as I'm concerned there's nothing "rough around the edges" that I would like fixed. I've done the "LGTM" thing about 10 times already on this review. Which is a PR that we're all waiting for. But changes are being consistently suggested for this first release of a command line tool. I was trying to suggest (apparently badly) that we just get it in now and fix it when the pressure to get a release tagged is off. Because, even if it will be used in production immediately: command line tool. Yes it may blow up (and emit a stackdump). Yes the command order might not match what's already there. And so on. But command line tool not 24/7/365 6-nines service. That's all I meant.

david-bakin-sl
david-bakin-sl approved these changes Apr 28, 2023
View reviewed changes
Copy link
Member

@david-bakin-sl david-bakin-sl left a comment

Choose a reason for hiding this comment

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

LGTM one more time for luck!

@iwsimon iwsimon closed this Apr 28, 2023
@povolev15 povolev15 reopened this Apr 28, 2023
@david-bakin-sl
Copy link
Member

david-bakin-sl commented Apr 28, 2023 •
edited

(Oh thank goodness that was a bad button press!) (I've done it myself...)

@sonarcloud
Copy link

sonarcloud bot commented Apr 28, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 6 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@povolev15 povolev15 merged commit 1a0303f into develop Apr 28, 2023
15 of 18 checks passed
@povolev15 povolev15 deleted the 5820-sign-services branch April 28, 2023 14:35
@nickpoorman nickpoorman mentioned this pull request May 1, 2023
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@povolev15 povolev15 povolev15 approved these changes

@JeffreyDallas JeffreyDallas JeffreyDallas approved these changes

@qnswirlds qnswirlds qnswirlds approved these changes

@david-bakin-sl david-bakin-sl david-bakin-sl approved these changes

@nathanklick nathanklick Awaiting requested review from nathanklick nathanklick is a code owner automatically assigned from hashgraph/release-engineering-managers

@nickpoorman nickpoorman Awaiting requested review from nickpoorman nickpoorman is a code owner automatically assigned from hashgraph/hedera-services

@kimbor kimbor Awaiting requested review from kimbor

@tinker-michaelj tinker-michaelj Awaiting requested review from tinker-michaelj

@mhess-swl mhess-swl Awaiting requested review from mhess-swl

@netopyr netopyr Awaiting requested review from netopyr netopyr was automatically assigned from hashgraph/platform-base

@cody-littley cody-littley Awaiting requested review from cody-littley

@alittley alittley Awaiting requested review from alittley

@Neeharika-Sompalli Neeharika-Sompalli Awaiting requested review from Neeharika-Sompalli

@jeromy-cannon jeromy-cannon Awaiting requested review from jeromy-cannon

Assignees

@iwsimon iwsimon

Labels
None yet
Projects
Services Sprint Tracking
Archived in project
Milestone
v0.38
Development

Successfully merging this pull request may close these issues.

Add PCLI command to sign account balance files
9 participants
@iwsimon @povolev15 @nickpoorman @david-bakin-sl @jeromy-cannon @JeffreyDallas @qnswirlds @Neeharika-Sompalli @alittley