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

refactor: align object names with KERIpy and add function docs #307

Merged
merged 6 commits into from
Feb 14, 2025

Conversation

kentbull
Copy link
Collaborator

@kentbull kentbull commented Jan 27, 2025

This is a work in progress PR. Status will be updated to "Ready for Review" at the end of this week.

Goals of PR

  • Reduce cognitive overhead in comparing SignifyTS code to KERIpy code by aligning class, interface, function, and argument names with KERIpy.
  • Add documentation to everything involved in Signify key management.
  • Small refactorings to leverage type safety of Typescript's static typing where there is low hanging fruit for easy wins.

Overview

This PR does three main things:

  • Aligns names with KERIpy where possible or practical.
  • Adds function, class, and method documentation in many places.
  • Renames things like Keeper, SaltyKeeper, and RandyKeeper to names that more closely match the KERIpy implementation.

Example - Keeper to IdentifierManager

As an example, key management in KERIpy occurs in keeping.Manager, not keeping.Keeper, so the SignifyTS Keeper interface, which mimicked the keeping.Manager class in KERIpy, was renamed to be IdentifierManager. The keeping.Keeper class in KERIpy was used to store keys in the LMDB database whereas the Manager was used for creating keypairs, performing inception, rotation, and signing, among other responsibilities not ported to SignifyTS from KERIpy.

The name Manager is already taken in SignifyTS by the manager.Manager class, yet the responsibilities of the keeping.Manager class were being split across both the manager.Manager class and the keeping.Keeper class. So the existing keeping.Keeper class in SignifyTS was renamed to keeping.IdentifierManager since that seemed to be a descriptive, clear name describing the fact that implementations of this object perform key pair creation, inception, rotation, and signing.

Motivation

While reading the Signify source code recently to gain a deeper understanding of key management in Signify I have noticed again a number of naming differences between SignifyTS and KERIpy that are unnecessary and introduce questions of meaning and compatibility. This PR makes the code clear by aligning the codebase with the naming used in the KERIpy codebase.

The significant work of Phil and Kevin, and those who came after them, to port the style, names, and ideas from KERIpy is a strong influence on the motivation to continue that alignment and further it.

Future work to complete this PR

My current task is to understand the ways stems for Salty key generation and lookup work. I plan to document everything I read as I gain a complete understanding of that code.

Looking for comments

Please feel free to opine and share your thoughts on what naming policies or related ideas should make it into this PR.

@kentbull kentbull force-pushed the enhance-docs-and-tests branch from a6965fe to 482c023 Compare January 27, 2025 05:01
@kentbull
Copy link
Collaborator Author

Another goal of this PR is to get ready to add CESR 2.0 and KERI 2.0 support.

@kentbull kentbull force-pushed the enhance-docs-and-tests branch 2 times, most recently from 83ea85c to 0ddf614 Compare January 27, 2025 05:15
@kentbull kentbull force-pushed the enhance-docs-and-tests branch from 0ddf614 to 5996d11 Compare January 27, 2025 05:18
Copy link

codecov bot commented Jan 27, 2025

Codecov Report

Attention: Patch coverage is 96.02649% with 6 lines in your changes missing coverage. Please review.

Project coverage is 83.70%. Comparing base (1e451d1) to head (6cf27c2).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/keri/core/keeping.ts 94.23% 3 Missing ⚠️
src/keri/core/core.ts 93.33% 1 Missing ⚠️
src/keri/core/eventing.ts 85.71% 1 Missing ⚠️
src/keri/core/serder.ts 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #307      +/-   ##
==========================================
+ Coverage   83.64%   83.70%   +0.05%     
==========================================
  Files          48       48              
  Lines        4238     4270      +32     
  Branches     1055     1067      +12     
==========================================
+ Hits         3545     3574      +29     
- Misses        663      666       +3     
  Partials       30       30              

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

@edeykholt
Copy link

Looks like great goals and work!
Since signify-browser-extension has a dependency on some of the exports of signify-ts main branch (as do others outside of WebOfTrust), and those exports might be changed with this commit, I'd like to see one of the following: 1) reassurance that signify-browser-extension is not affected; 2) signify-browser-extension is kept in sync as needed with a corresponding pull request; 3) signify-browser-extension is updated to reference a specific version (or commit hash) in its package.json; or 4) this commit happens on a signify-ts branch until these series of expected changes settle.

@kentbull
Copy link
Collaborator Author

Good points Ed. I'll take a look at the signify-browser-extension code to see what exports it depends on.

@kentbull
Copy link
Collaborator Author

@edeykholt item 3 of your notes above is addressed here: WebOfTrust/signify-browser-extension#229

@kentbull
Copy link
Collaborator Author

And @edeykholt , to your first point

  1. reassurance that signify-browser-extension is not affected;

the results of a full text search on the signify-browser-extension repository for the names I changed here show that nothing in the browser extension depends on the names I changed here.

I will keep this in mind as I make further changes this week. If there are any refactorings that affect the browser extension then I will create a corresponding PR to that repo.

@kentbull kentbull marked this pull request as ready for review February 4, 2025 20:22
@kentbull kentbull force-pushed the enhance-docs-and-tests branch from a4f60d2 to da4f5c3 Compare February 4, 2025 20:24
@kentbull kentbull requested review from lenkan and 2byrds February 5, 2025 15:30
Copy link
Collaborator

@lenkan lenkan left a comment

Choose a reason for hiding this comment

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

Great work with all the cleanup and comments!

I added some minor nitpicking. But mostly, I am not sure about the IdentifierManager vs KeyManager naming. See the comment in the keeping.ts file. I read your rationale, but maybe I misunderstood.

@kentbull kentbull force-pushed the enhance-docs-and-tests branch from 500fa11 to 5cc1e3c Compare February 7, 2025 01:25
@kentbull
Copy link
Collaborator Author

kentbull commented Feb 7, 2025

@lenkan thanks for the comments! I made corresponding changes or added rationale for context.

@kentbull kentbull force-pushed the enhance-docs-and-tests branch from bece40e to 6cf27c2 Compare February 13, 2025 19:53
Copy link
Contributor

@2byrds 2byrds left a comment

Choose a reason for hiding this comment

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

Thank you for responding to the feedback. I hope this achieves the alignment attended. Time will tell i suppose, but glad that we are trying :)

@kentbull kentbull merged commit dbffa56 into WebOfTrust:main Feb 14, 2025
8 checks passed
@kentbull kentbull deleted the enhance-docs-and-tests branch February 14, 2025 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants