Skip to content

Conversation

@Sotatek-DucPhung
Copy link
Collaborator

No description provided.

@Sotatek-DucPhung Sotatek-DucPhung marked this pull request as draft September 4, 2025 09:14
@iFergal iFergal self-requested a review September 9, 2025 19:27
Copy link
Collaborator

@iFergal iFergal left a comment

Choose a reason for hiding this comment

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

@Sotatek-DucPhung Feel free to reply to my comments, but latest priorities as shared on Slack take priority tomorrow. Patrick can maybe pick this up later.

@iFergal iFergal marked this pull request as ready for review November 27, 2025 09:43
@iFergal
Copy link
Collaborator

iFergal commented Nov 27, 2025

Not sure why one of the e2e tests is failing

Copy link
Collaborator

@iFergal iFergal left a comment

Choose a reason for hiding this comment

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

I have not individually reviewed the generated classes as there are too many.

Our goal is to start leveraging these types to replace Object throughout the repository - this may highlight issues in itself - but I will also start reviewing the used types batch by batch in each PR. Subsequent PRs will be small to be reviewable.

The risk should be somewhat low anyway as these types are already in use by Signify-TS (or starting to be in use).

For further security reasons, I can do a full regeneration of the types later when all subsequent PRs have landed.

@iFergal iFergal changed the title chore: gen class chore: generate API level classes from KERIA OpenAPI Nov 27, 2025
@iFergal iFergal merged commit 5a9ba5b into main Nov 27, 2025
3 checks passed
@iFergal iFergal deleted the chore/generate-types-from-keria branch November 27, 2025 09:59
Copy link
Collaborator

Choose a reason for hiding this comment

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

Checked files used from PR #64

  • It seems a default (Java here and TS here) causes @nullable - do you know why that would be?
  • I noticed transferable and windexes have JsonNullable - Signify-TS also has | null in the type union…
    • Can you check with @Sotatek-Patrick-Vu here on why those might be nullable like that - I don’t think they should be?
Checked:
----------

- IdentifiersPostRequest
- IdentifiersNamePutRequest
- EndrolesAidPostRequest
- StateEERecord
- Identifier
- Tier
- KeyStateRecord
- RandyKeyState
- SaltyState
- GroupKeyState

Copy link
Collaborator

Choose a reason for hiding this comment

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

@iFergal for transferable and windexes they can be optional but not nullable. I think we need to change the type in Keria for them then Java and TS types will be auto updated
you can see here: https://github.com/WebOfTrust/keria/blob/54611c911b8bf5b2606e170fb85c2f5854b089eb/src/keria/app/aiding.py#L1252

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Sotatek-Patrick-Vu That's true - but it's always called with full=True so they should always be provided, right?

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