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

chore: account init to update node admin key #1373

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

JeffreyDallas
Copy link
Contributor

Description

This pull request changes the following:

  • account init to update node admin key

Related Issues

Signed-off-by: Jeffrey Tang <[email protected]>
Signed-off-by: Jeffrey Tang <[email protected]>

# Conflicts:
#	src/commands/account.ts
@JeffreyDallas JeffreyDallas requested review from leninmehedy and a team as code owners February 12, 2025 15:36
@JeffreyDallas JeffreyDallas marked this pull request as draft February 12, 2025 15:36
Copy link
Contributor

github-actions bot commented Feb 12, 2025

Unit Test Results - Linux

  1 files  ±0   62 suites  ±0   2s ⏱️ -1s
241 tests ±0  241 ✅ ±0  0 💤 ±0  0 ❌ ±0 
250 runs  ±0  250 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 457f2a9. ± Comparison against base commit 370f57b.

♻️ This comment has been updated with latest results.

Copy link
Contributor

github-actions bot commented Feb 12, 2025

Unit Test Results - Windows

  1 files  ±0   62 suites  ±0   2s ⏱️ -1s
241 tests ±0  241 ✅ ±0  0 💤 ±0  0 ❌ ±0 
250 runs  ±0  250 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 457f2a9. ± Comparison against base commit 370f57b.

♻️ This comment has been updated with latest results.

Copy link
Contributor

github-actions bot commented Feb 12, 2025

E2E Test Report

 17 files  126 suites   1h 28m 27s ⏱️
255 tests 255 ✅ 0 💤 0 ❌
266 runs  266 ✅ 0 💤 0 ❌

Results for commit 1a48a93.

♻️ This comment has been updated with latest results.

Copy link

codacy-production bot commented Feb 12, 2025

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
+0.01% (target: -1.00%) 100.00%
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (8411ff3) 23707 19815 83.58%
Head commit (1a48a93) 23721 (+14) 19829 (+14) 83.59% (+0.01%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#1373) 15 15 100.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more

Copy link

codecov bot commented Feb 12, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.52%. Comparing base (369065e) to head (1a48a93).
Report is 3 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1373      +/-   ##
==========================================
+ Coverage   82.47%   82.52%   +0.04%     
==========================================
  Files         130      130              
  Lines       23642    23721      +79     
  Branches     2366     1493     -873     
==========================================
+ Hits        19498    19575      +77     
- Misses       3904     4133     +229     
+ Partials      240       13     -227     
Files with missing lines Coverage Δ
src/commands/account.ts 76.08% <100.00%> (+0.51%) ⬆️

... and 56 files with indirect coverage changes

Impacted file tree graph

@JeffreyDallas JeffreyDallas marked this pull request as ready for review February 12, 2025 17:48
const newPrivateKey = PrivateKey.generateED25519();
const genesisAccountId = AccountId.fromString(constants.FREEZE_ADMIN_ACCOUNT);
// update genesis account key
await self.accountManager.updateAccountKeys(
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not what was meant. We need to update the Node Admin Keys. I believe the only way to do this is via the NodeUpdateTransaction:

self.logger.info(`config.tlsPublicKey: ${config.tlsPublicKey}`);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

Copy link
Contributor

@jeromy-cannon jeromy-cannon left a comment

Choose a reason for hiding this comment

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

Need to use NodeUpdateTransaction to update each of the node's admin keys.

@jeromy-cannon jeromy-cannon added the PR: Unresolved Comments A pull request where there are comments and they need to be resolved. label Feb 12, 2025
@jeromy-cannon jeromy-cannon added the PR: Merge Conflicts A pull request that has merge conflicts that need to be resolved. label Feb 14, 2025
Signed-off-by: Jeffrey Tang <[email protected]>
…node-adminkey

Signed-off-by: Jeffrey Tang <[email protected]>

# Conflicts:
#	src/commands/account.ts
#	src/commands/node/tasks.ts
Signed-off-by: Jeffrey Tang <[email protected]>
Signed-off-by: Jeffrey Tang <[email protected]>
@JeffreyDallas JeffreyDallas added PR: Needs Approval A pull request that needs reviews and approvals. and removed PR: Merge Conflicts A pull request that has merge conflicts that need to be resolved. labels Feb 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Needs Approval A pull request that needs reviews and approvals. PR: Unresolved Comments A pull request where there are comments and they need to be resolved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Production Readiness: Update account init/rekey logic to also update the Node admin keys
2 participants