Skip to content

Build: Use latest oapi-codegen instead of fork. #6306

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

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

jannotti
Copy link
Contributor

@jannotti jannotti commented Apr 21, 2025

Summary

We forked oapi-codegen, but it appears that we can get everything we want without that fork, and it would be nice to maintain one less fork and be able to use new features of the tool.

We had added a type mapping feature so that anything with the openapi type integer was declared in our models as uint64. Since then (or maybe we had missed it), oapi-codegen has support so that such fields can be marked x-go-type: uint64 and get the same effect.

We had also added support to allow middleware to be specified when registering echo handlers by overriding the template used to generate the registry functions.

While inserting x-go-type, it became clear that we could go further and use x-go-type: basics.Round and a few other nice internal types. This will only affect the code generated for the server, so SDKs will not need those types. Carrying that through on as many fields as possible created a lot of changes, but mostly simplistic. A number of places got simpler and more clear because we were using uint64 in many places on our code that had to be compatible with the generated models. Even though I added a lot of x-go-type lines, the PR as a whole drops 800 lines. (And many remaining lines are simpler, with less casting.)

It's worth mentioning that I first tried using format: uint64. That mostly worked, but it caused our SDK generator to generate BigInteger (since Java long is really only 63 bits of positive number). format also would not have supported the fine-grained Go types from basics.

Test Plan

Existing tests should pass. Generated SDKs should be unchanged.

Had to resolve a funny issue of not generating some types. Resolved by
moving teh definitions to $refs.

Upstream can't take middleware though.  Maybe they'll take PR?
@jannotti jannotti self-assigned this Apr 21, 2025
@jannotti jannotti requested a review from Copilot April 21, 2025 17:35
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates our codebase to use the latest oapi-codegen implementation instead of our fork. The key changes include updating the enum conversion in the Go handler and adding comprehensive "uint64" format annotations throughout our OpenAPI schema to leverage new features from oapi-codegen.

Reviewed Changes

Copilot reviewed 19 out of 25 changed files in this pull request and generated no comments.

File Description
daemon/algod/api/server/v2/handlers.go Updated the enum conversion for the transaction proof hash type to match the latest oapi-codegen.
daemon/algod/api/algod.oas3.yml Added extensive "format": "uint64" annotations and replaced an inline definition with a $ref.
Files not reviewed (6)
  • daemon/algod/api/Makefile: Language not supported
  • daemon/algod/api/templates/echo/echo-register.tmpl: Language not supported
  • go.mod: Language not supported
  • scripts/buildtools/install_buildtools.sh: Language not supported
  • scripts/buildtools/versions: Language not supported
  • tools/block-generator/go.mod: Language not supported
Comments suppressed due to low confidence (2)

daemon/algod/api/server/v2/handlers.go:949

  • The update from 'TransactionProofResponseHashtype' to 'TransactionProofHashtype' aligns with the latest oapi-codegen usage; please verify that all dependent code and type conversions have been updated accordingly.
Hashtype:  model.TransactionProofHashtype(hashtype),

daemon/algod/api/algod.oas3.yml:52

  • The addition of the 'format': 'uint64' annotation throughout the schema appears consistent with the new oapi-codegen features; ensure that these annotations are fully supported by downstream consumers and serialization processes.
"format": "uint64",

@jannotti jannotti force-pushed the oapi-without-typemapping branch 3 times, most recently from 7669418 to c16fcbf Compare April 21, 2025 18:07
Copy link

codecov bot commented Apr 21, 2025

Codecov Report

Attention: Patch coverage is 25.93750% with 474 lines in your changes missing coverage. Please review.

Project coverage is 51.91%. Comparing base (f13575c) to head (ce92401).

Files with missing lines Patch % Lines
daemon/algod/api/server/v2/handlers.go 0.00% 108 Missing ⚠️
...ver/v2/generated/nonparticipating/public/routes.go 0.00% 82 Missing ⚠️
libgoal/transactions.go 0.00% 33 Missing ⚠️
libgoal/libgoal.go 13.88% 31 Missing ⚠️
cmd/goal/account.go 41.17% 20 Missing ⚠️
daemon/algod/api/client/restClient.go 0.00% 19 Missing ⚠️
...erver/v2/generated/participating/private/routes.go 5.00% 19 Missing ⚠️
daemon/algod/api/server/v2/utils.go 0.00% 17 Missing ⚠️
...er/v2/generated/nonparticipating/private/routes.go 0.00% 14 Missing ⚠️
shared/pingpong/accounts.go 0.00% 14 Missing ⚠️
... and 27 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6306      +/-   ##
==========================================
+ Coverage   51.84%   51.91%   +0.07%     
==========================================
  Files         652      652              
  Lines       87442    87367      -75     
==========================================
+ Hits        45331    45357      +26     
+ Misses      39244    39149      -95     
+ Partials     2867     2861       -6     

☔ 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.

I think everything we do in our fork can be done with the current
oapi-codegen, so it is nice to maintain one less fork and have our
tool stay up to date.
@jannotti jannotti force-pushed the oapi-without-typemapping branch from c16fcbf to 57c1485 Compare April 21, 2025 18:51
@jannotti jannotti marked this pull request as ready for review April 21, 2025 20:46
@jannotti jannotti force-pushed the oapi-without-typemapping branch from 5f577cd to 87ee95b Compare April 22, 2025 15:19
@jannotti jannotti requested a review from gmalouf April 23, 2025 19:41
algorandskiy
algorandskiy previously approved these changes Apr 25, 2025
@jannotti jannotti requested review from algorandskiy and cce May 5, 2025 18:27
@jannotti jannotti marked this pull request as draft May 7, 2025 02:11
@jannotti jannotti force-pushed the oapi-without-typemapping branch 3 times, most recently from 44d4ec3 to c704f34 Compare May 9, 2025 19:23
@jannotti jannotti force-pushed the oapi-without-typemapping branch from c704f34 to 0e959a4 Compare May 9, 2025 20:17
@jannotti jannotti requested a review from Copilot May 9, 2025 20:22
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR migrates our codebase away from a forked version of oapi‑codegen by taking advantage of new features such as native support for the format: uint64 directive and updated type mappings. Key changes include:

  • Replacing custom type definitions based on uint64 with the appropriate types defined in basics (e.g. basics.Round, basics.AppIndex, basics.AssetIndex).
  • Updating command‐line flag bindings to explicitly cast pointers to uint64 for compatibility.
  • Refining message packing and unmarshalling logic (e.g. using MsgIsZero() methods) in generated code to match upstream expectations.

Reviewed Changes

Copilot reviewed 133 out of 133 changed files in this pull request and generated no comments.

Show a summary per file
File Description
cmd/loadgenerator/config.go Updated RoundModulator and RoundOffset types to basics.Round
cmd/goal/node.go Replaced manual nil pointer handling with helper nilToZero
cmd/goal/ledger.go Cast round values to basics.Round in client.RawBlock call
cmd/goal/interact.go Adjusted flag parsing for app-id using pointer casts
cmd/goal/formatting.go Introduced a generic nilToZero helper function
cmd/goal/common.go Changed first/last valid round variables to basics.Round
cmd/goal/clerk.go Changed simulateExtraOpcodeBudget type from uint64 to int
cmd/goal/box.go, cmd/goal/asset.go, ... Updated flag bindings and type conversions consistently
cmd/goal/application.go Updated appIdx and reference argument handling with basics types
cmd/goal/account.go Adjusted flag parsing for round values using pointer casts
cmd/algoh/* Updated mock client, deadman watcher, client methods, and blockwatcher to use basics types
catchup/service.go Updated synchronization round functions to use basics.Round
agreement/msgp_gen.go Replaced raw zero comparisons with .MsgIsZero() checks in msgp code
Comments suppressed due to low confidence (2)

cmd/goal/clerk.go:76

  • Changing simulateExtraOpcodeBudget from uint64 to int could introduce issues if the budget value exceeds the range of int. Please confirm that this conversion is safe and intentional.
simulateExtraOpcodeBudget     int

agreement/msgp_gen.go:4538

  • Using MsgIsZero() to check for a zero value ensures more robust handling of upgraded types compared to a raw numeric comparison. Verify that this change correctly reflects the intended behavior.
if (*z).unauthenticatedProposal.Block.BlockHeader.UpgradeState.NextProtocolApprovals.MsgIsZero() {

With x-go-type we can also explicitly make Rounds, Assets, and Apps
use their types from `basics`.  Having done so, a LOT of code can be
cahnged to use those types properly instead of propagating uint64s
throughout.
@jannotti jannotti marked this pull request as ready for review May 10, 2025 00:46
@jannotti jannotti force-pushed the oapi-without-typemapping branch from 0e959a4 to 97df962 Compare May 10, 2025 13:53
@jannotti jannotti force-pushed the oapi-without-typemapping branch 3 times, most recently from 3dc14d5 to be9cfce Compare May 12, 2025 13:03
@jannotti jannotti force-pushed the oapi-without-typemapping branch from be9cfce to ce92401 Compare May 12, 2025 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants