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 admin key to get /network/nodes endpoint #9776

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

Conversation

sdimitrov9
Copy link

@sdimitrov9 sdimitrov9 commented Nov 18, 2024

Description:
Adding admin_key to the get /network/nodes REST API

Related issue(s):

Fixes #8757

Notes for reviewer:
Screenshot 2024-11-18 at 11 38 06
Screenshot 2024-11-18 at 11 39 14

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)
Screenshot 2024-11-18 at 11 43 48

@sdimitrov9 sdimitrov9 added the rest Area: REST API label Nov 18, 2024
@sdimitrov9 sdimitrov9 self-assigned this Nov 18, 2024
@sdimitrov9 sdimitrov9 requested a review from a team as a code owner November 18, 2024 09:42
Copy link

codecov bot commented Nov 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.25%. Comparing base (0c38f14) to head (66f2822).
Report is 18 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #9776      +/-   ##
============================================
+ Coverage     92.23%   92.25%   +0.02%     
- Complexity     7628     7769     +141     
============================================
  Files           937      951      +14     
  Lines         32117    32467     +350     
  Branches       4071     4113      +42     
============================================
+ Hits          29622    29953     +331     
- Misses         1542     1546       +4     
- Partials        953      968      +15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@sdimitrov9 sdimitrov9 added the enhancement Type: New feature label Nov 18, 2024
@SvetBorislavov
Copy link

SvetBorislavov commented Nov 18, 2024

Hey, the 'admin_key' value structure is not correct. The node's admin_key is of type 'Key', here is the definition inside the protobuf: https://github.com/hashgraph/hedera-protobufs/blob/main/services/state/addressbook/node.proto#L148
The mirror node currently returns this kind of 'Key' structure in the following format:

"key": {
        "_type": "ED25519",
        "key": "61f37fc1bbf3ff4453712ee6a305c5c7255955f7889ec3bf30426f1863158ef4"
      }

This is an example of the /accounts response:
Here is the whole request: https://testnet.mirrornode.hedera.com/api/v1/accounts?account.id=2159149

The structure should be such because the key may be a KeyList with multiple levels.

The key type is also shown in the screenshot you have attached.

@sdimitrov9 sdimitrov9 force-pushed the 8757-add-admin-key-to-get-nodes-endpoint branch from 7415cf2 to 54dda1f Compare November 18, 2024 14:58
Signed-off-by: sdimitrov9 <stoyan.dimitrov@limechain.tech>
@sdimitrov9 sdimitrov9 force-pushed the 8757-add-admin-key-to-get-nodes-endpoint branch from 54dda1f to fe35df4 Compare November 18, 2024 15:09
… API

Signed-off-by: sdimitrov9 <stoyan.dimitrov@limechain.tech>
@sdimitrov9 sdimitrov9 force-pushed the 8757-add-admin-key-to-get-nodes-endpoint branch from fe35df4 to ede7fd4 Compare November 18, 2024 15:16
Signed-off-by: sdimitrov9 <stoyan.dimitrov@limechain.tech>
@sdimitrov9 sdimitrov9 force-pushed the 8757-add-admin-key-to-get-nodes-endpoint branch from ede7fd4 to bacbf07 Compare November 18, 2024 15:17
@sdimitrov9 sdimitrov9 marked this pull request as draft November 18, 2024 15:24
@steven-sheehy steven-sheehy added this to the 0.119.0 milestone Nov 18, 2024
hedera-mirror-rest/viewmodel/networkNodeViewModel.js Outdated Show resolved Hide resolved
hedera-mirror-rest/model/node.js Outdated Show resolved Hide resolved
hedera-mirror-rest/model/node.js Outdated Show resolved Hide resolved
hedera-mirror-rest/api/v1/openapi.yml Outdated Show resolved Hide resolved
hedera-mirror-rest/api/v1/openapi.yml Outdated Show resolved Hide resolved
Signed-off-by: sdimitrov9 <stoyan.dimitrov@limechain.tech>
@sdimitrov9 sdimitrov9 marked this pull request as ready for review November 19, 2024 14:57
@sdimitrov9 sdimitrov9 requested a review from a team November 19, 2024 14:57
Signed-off-by: sdimitrov9 <stoyan.dimitrov@limechain.tech>
Copy link

sonarcloud bot commented Nov 19, 2024

@steven-sheehy steven-sheehy changed the title 8757 add admin key to get /network/nodes endpoint Add admin key to get /network/nodes endpoint Nov 19, 2024
@@ -52,8 +52,13 @@ describe('NetworkNodeViewModel', () => {
stakeRewarded: 4,
stakingPeriod: '1654991999999999999',
},
node: {
adminKey: null,
Copy link
Member

Choose a reason for hiding this comment

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

The test should populate a valid key

@@ -367,6 +367,7 @@
"responseJson": {
"nodes": [
{
"admin_key": null,
Copy link
Member

Choose a reason for hiding this comment

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

All specs have the admin_key as null. We should populate valid admin keys for at least one file like this one. It should include different variations like ED25519, ECDSA, 3200 immutable key, and one complex key.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Type: New feature rest Area: REST API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HIP-869 Add admin_key to the /network/nodes REST API
3 participants