Skip to content

Adding type hints for other End files delegating.py, grouping.py, ipexing.py, notifying.py, exchanging.py#355

Merged
iFergal merged 13 commits intoWebOfTrust:mainfrom
cardano-foundation:feat/common_type_hints
Mar 10, 2026
Merged

Adding type hints for other End files delegating.py, grouping.py, ipexing.py, notifying.py, exchanging.py#355
iFergal merged 13 commits intoWebOfTrust:mainfrom
cardano-foundation:feat/common_type_hints

Conversation

@Sotatek-Patrick-Vu
Copy link
Copy Markdown
Contributor

No description provided.

@codecov
Copy link
Copy Markdown

codecov Bot commented Dec 10, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.53%. Comparing base (5e540e7) to head (6b3cef3).
⚠️ Report is 8 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #355   +/-   ##
=======================================
  Coverage   97.53%   97.53%           
=======================================
  Files           1        1           
  Lines         324      325    +1     
  Branches       24       24           
=======================================
+ Hits          316      317    +1     
  Misses          8        8           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment thread src/keri/app/credentialing.ts
Comment thread test-integration/credentials.test.ts Outdated
@iFergal iFergal requested review from kentbull and lenkan December 18, 2025 17:02
@kentbull
Copy link
Copy Markdown
Collaborator

@iFergal feel free to approve and merge.

@iFergal
Copy link
Copy Markdown
Collaborator

iFergal commented Jan 14, 2026

@kentbull Going to see if there's a nice way to serialise the exn resources from exchanges().get and infer types based on the route. Because currently embeds are unknown and it's difficult to work with. You need to cast everything, so Signify should at the very least provide those.

@Sotatek-Patrick-Vu Maybe we should mark as draft for a little while

Copy link
Copy Markdown
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.

Looks good to me.

It introduces a bit too much type casting and type checking in the tests for my taste. But I get how that happened with all the re-assigning of variables, which is a separate problem.

Comment thread src/types/keria-api-schema.ts Outdated
Comment on lines +732 to +737
/** @default null */
groupName: string | null;
/** @default null */
memberName: string | null;
/** @default null */
sender: string | null;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can these be omitted? Or they have to be explicitly set to null? Was thinking they are groupName?: string | null otherwise.

I haven't checked how they are used yet.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

KERIA adds them to some exns in groups().getRequest. I think we could change it to be groupName?: string; over groupName: string | null though

Copy link
Copy Markdown
Contributor Author

@Sotatek-Patrick-Vu Sotatek-Patrick-Vu Feb 11, 2026

Choose a reason for hiding this comment

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

I fixed this then we also need to change relevant dataclass in keria

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I created relevant PR in Keria here WebOfTrust/keria#417

Comment thread test-integration/singlesig-drt.test.ts Outdated
@iFergal
Copy link
Copy Markdown
Collaborator

iFergal commented Jan 27, 2026

@lenkan The type casting is because of unknown everywhere. We have been working on a follow up PR which is almost ready to be opened against this repo: https://github.com/cardano-foundation/signify-ts/pull/22/changes

Feel free to take a look ahead of time. I'd like for both of them to go in together, as I also don't like the type casting.

@iFergal iFergal requested review from kentbull and lenkan February 11, 2026 14:29
@iFergal
Copy link
Copy Markdown
Collaborator

iFergal commented Feb 11, 2026

@lenkan @kentbull Ready for re-review.

Notes:

  • Type guards here help avoid casting on embeds.
    • In some cases, we still have casting for things that we can't predict generically, like what the seal format for an interaction event will be. Maybe in the future we can provide some more helpers for common ones.
    • For now, we are adding types like MultisigRpyExchange here but we can try to move these to KERIA too so they are re-usable, in another PR.
  • The assert functions are useful in the tests or when the application knows what to expect, so it throws if it's not what is expected. More clever architectures might just route to various handles based on the route value.
  • As mentioned before, we have a subsequent PR to fix the operation typing (which is unknown right now).

Copy link
Copy Markdown
Collaborator

@kentbull kentbull left a comment

Choose a reason for hiding this comment

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

Almost there. Just one more field change and you're done.

Comment thread src/keri/app/exchanging.ts
Comment thread src/types/keria-api-schema.ts
Copy link
Copy Markdown
Collaborator

@kentbull kentbull left a comment

Choose a reason for hiding this comment

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

LGTM

@iFergal
Copy link
Copy Markdown
Collaborator

iFergal commented Mar 10, 2026

Merging as this has been open for quite some time and is safe enough to merge. If there are more comments on the approach, an issue can be opened.

@iFergal iFergal merged commit 4c293bb into WebOfTrust:main Mar 10, 2026
10 checks passed
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