Add context helpers for actor keys by usage#604
Add context helpers for actor keys by usage#604SergioChan wants to merge 1 commit intofedify-dev:mainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces new utility methods within the federation context to streamline the retrieval of actor cryptographic keys. By providing helpers to access public keys and assertion methods based on their usage, it simplifies the logic for components like the actor dispatcher, reducing the need for manual key pair processing and enhancing code clarity. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
issues-auto-labeler
bot
added
component/actor
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces two new helper methods, getActorKeysByUsage and getActorFirstKeyByUsage, to the Context for easier access to actor keys. The implementations and tests are well-structured. I've suggested one optimization for getActorFirstKeyByUsage to improve its efficiency by avoiding the creation of unnecessary intermediate arrays.
| async getActorFirstKeyByUsage(identifier: string): Promise<{ | ||
| publicKey: CryptographicKey | undefined; | ||
| assertionMethod: Multikey | undefined; | ||
| }> { | ||
| const { publicKeys, assertionMethods } = | ||
| await this.getActorKeysByUsage(identifier); | ||
| return { | ||
| publicKey: publicKeys[0], | ||
| assertionMethod: assertionMethods[0], | ||
| }; | ||
| } |
There was a problem hiding this comment.
This implementation is correct, but it can be made more efficient. It currently calls getActorKeysByUsage, which creates two new arrays by mapping over all key pairs, only to then use the first element of each. You can optimize this by calling getActorKeyPairs directly and accessing the first element, avoiding the creation of intermediate arrays.
async getActorFirstKeyByUsage(identifier: string): Promise<{
publicKey: CryptographicKey | undefined;
assertionMethod: Multikey | undefined;
}> {
const keys = await this.getActorKeyPairs(identifier);
const firstKey = keys[0];
if (firstKey == null) {
return { publicKey: undefined, assertionMethod: undefined };
}
return {
publicKey: firstKey.cryptographicKey,
assertionMethod: firstKey.multikey,
};
}
|
Thank you so much for taking the time to implement this—I'm sorry you put in the effort before we had a chance to weigh in on the issue. Unfortunately, we've just closed the related issue #488 as “not planned.” The main reasons are:
I'm closing this PR accordingly. Again, I really appreciate your contribution, and I hope this doesn't discourage you from contributing in the future—please feel free to open a discussion first for larger changes so we can align early. |
2 participants
Summary
Add Context helpers to retrieve actor public keys and assertion methods by usage, plus a convenience method for the first key.
Related issue
Context.getActorFirstKeyByUsage(id)andContext.getActorKeysByUsage(id). #488Changes
Benefits
Simplifies actor dispatcher code by avoiding manual key pair mapping for public keys and assertion methods.
Checklist
mise teston your machine?Additional notes
Tests not run (not requested).