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

Expose ed25519 key explicitly and remove admin_key in t_entities #217

Merged
merged 2 commits into from
Sep 9, 2019

Conversation

mike-burrage-hedera
Copy link
Contributor

@mike-burrage-hedera mike-burrage-hedera commented Sep 7, 2019

Detailed description:

key column will now be used in place of admin_key where admin_key was used in the past. No entity had both

  • copied data from t_entities.admin_key to t_entities.key if present
  • deprecated t_entities.admin_key
  • added t_entities.ed25519_public_key_hex (lowercase hex string if the key)
  • updated java, sql function, and rest-api for this change
  • rest-api will no longer be returning admin_key publicly (didn't make sense on accounts anyway)

Which issue(s) this PR fixes:
Fixes #56

Special notes for your reviewer:

Checklist

  • Documentation added
  • Tests updated

@mike-burrage-hedera mike-burrage-hedera added enhancement Type: New feature P2 labels Sep 7, 2019
@mike-burrage-hedera mike-burrage-hedera added this to the 0.2.0 milestone Sep 7, 2019
@mike-burrage-hedera mike-burrage-hedera changed the title Expose ed25519 key explicitly and remove admin_key in t_entities WIP: Expose ed25519 key explicitly and remove admin_key in t_entities Sep 7, 2019
@mike-burrage-hedera mike-burrage-hedera changed the title WIP: Expose ed25519 key explicitly and remove admin_key in t_entities Expose ed25519 key explicitly and remove admin_key in t_entities Sep 7, 2019
Copy link
Contributor

@gregscullard gregscullard left a comment

Choose a reason for hiding this comment

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

Oddly I was thinking about the same thing last night !

See suggested changes, would also suggest adding a unit test including a valid key in upper case to test it comes out of utility.protobufKeyToHexIfEd25519OrNull in lowercase.

src/main/java/com/hedera/recordFileLogger/Entities.java Outdated Show resolved Hide resolved
src/main/java/com/hedera/recordFileLogger/Entities.java Outdated Show resolved Hide resolved
…ntities.ed25519_public_key_hex.

Signed-off-by: Mike Burrage <mike.burrage@hedera.com>
@mike-burrage-hedera
Copy link
Contributor Author

Can't have a unit test where utility.protobufKeyToHexIfEd25519OrNull() produces uppercase, since it always produces lowercase, but I updated the javadoc on it to make that explicit.

Signed-off-by: Mike Burrage <mike.burrage@hedera.com>
@gregscullard
Copy link
Contributor

Can't have a unit test where utility.protobufKeyToHexIfEd25519OrNull() produces uppercase, since it always produces lowercase, but I updated the javadoc on it to make that explicit.

I meant provide it an uppercase input, validate output is lowercase.

@mike-burrage-hedera
Copy link
Contributor Author

There is no uppercase or lowercase input - the input is raw binary bits of the protobuf that

@gregscullard gregscullard removed their assignment Sep 9, 2019
@steven-sheehy steven-sheehy merged commit f63b463 into master Sep 9, 2019
@delete-merged-branch delete-merged-branch bot deleted the 56-entity-key branch September 9, 2019 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Type: New feature P2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose entity keys in the t_entities_table
4 participants