Skip to content

algod: Sunset v1 handlers#4847

Merged
michaeldiamant merged 12 commits intoalgorand:masterfrom
algochoi:sunset-v1-algod
Dec 8, 2022
Merged

algod: Sunset v1 handlers#4847
michaeldiamant merged 12 commits intoalgorand:masterfrom
algochoi:sunset-v1-algod

Conversation

@algochoi
Copy link
Copy Markdown
Contributor

@algochoi algochoi commented Dec 1, 2022

Summary

This PR sunsets the v1 algod APIs.

Changes

  • Route v1 handlers to a http.StatusGone (410) response handler
  • Delete "v1" from /versions
  • Delete v1 handlers and tests
  • Delete swagger generation comment in router.go
  • Use v2client in e2e runner Python scripts
  • Delete v1 swagger.json file and generator script
  • Delete swagger generation step in Makefile

Test Plan

Router tests/Local node testing

Python SDK seems to handle the 410 error correctly. Circle CI Example.

@codecov
Copy link
Copy Markdown

codecov Bot commented Dec 1, 2022

Codecov Report

Merging #4847 (bcafc11) into master (ffa688a) will increase coverage by 0.84%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4847      +/-   ##
==========================================
+ Coverage   53.31%   54.16%   +0.84%     
==========================================
  Files         423      421       -2     
  Lines       53852    53044     -808     
==========================================
+ Hits        28713    28731      +18     
+ Misses      22854    22040     -814     
+ Partials     2285     2273      -12     
Impacted Files Coverage Δ
daemon/algod/api/server/router.go 13.88% <ø> (ø)
cmd/tealdbg/debugger.go 72.69% <0.00%> (-0.81%) ⬇️
ledger/testing/randomAccounts.go 56.00% <0.00%> (-0.62%) ⬇️
network/wsNetwork.go 64.74% <0.00%> (-0.18%) ⬇️
network/wsPeer.go 69.45% <0.00%> (ø)
daemon/algod/api/server/v1/handlers/handlers.go
ledger/acctupdates.go 69.48% <0.00%> (+0.48%) ⬆️
ledger/acctonline.go 78.81% <0.00%> (+0.51%) ⬆️
ledger/catchpointtracker.go 58.88% <0.00%> (+0.74%) ⬆️
ledger/tracker.go 75.10% <0.00%> (+0.84%) ⬆️
... and 4 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

"type": "integer"
}
},
"x-go-package": "github.com/algorand/go-algorand/daemon/algod/api/spec/v1"
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'm not sure what this was being used for in the v2 /params path spec, but it looked unnecessary so I deleted it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I also looked and I'm unsure how x-go-package is used. In addition to general googling (e.g. https://swagger.io/specification/#specification-extensions), I reviewed these sources without finding a reference:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As an additional sanity check, I confirmed Go SDK source code generation does not change.

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.

@algochoi algochoi marked this pull request as ready for review December 5, 2022 16:48
Copy link
Copy Markdown
Contributor

@winder winder left a comment

Choose a reason for hiding this comment

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

There is a bunch of code in router.go that adapts the handler library code into an echo register function. I suspect you can delete most of it.

It should also be possible to register a single handler for all endpoints that begin with /v2 instead of keeping all the existing routes.

@algochoi
Copy link
Copy Markdown
Contributor Author

algochoi commented Dec 6, 2022

@winder

It should also be possible to register a single handler for all endpoints that begin with /v2 instead of keeping all the existing routes.

I think this would also mean invalid v1 routes (e.g. /v1/INVALID/PATH) would also return the "410 Gone" message prompting the user to move to the v2 API. Perhaps I'm splitting hairs, but it looks no different from just deleting all the v1 endpoints and returning a 404 error in this case.

The approach in this PR is:

  • Route previously valid v1 endpoints to "410 Gone" error and return body suggesting user should use the v2 API
  • Route invalid v1 endpoints to "404" (no change).

I'm also open to returning the same error for all the v1 paths in favor of simplifying the routing here though.

@michaeldiamant michaeldiamant linked an issue Dec 7, 2022 that may be closed by this pull request
Comment thread daemon/algod/api/server/v1/routes/routes.go Outdated
Comment thread daemon/algod/api/algod.oas2.json
Comment thread Makefile
Comment thread Makefile
Copy link
Copy Markdown
Contributor

@michaeldiamant michaeldiamant left a comment

Choose a reason for hiding this comment

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

@algochoi Congrats on a net removal of 9,735 lines! ☕

Looks ready to me - Thanks for your effort across multiple PRs + tickets to help sunset algod v1 API in go-algorand.

@algochoi algochoi requested a review from winder December 7, 2022 19:35
@michaeldiamant michaeldiamant merged commit 0f66b33 into algorand:master Dec 8, 2022
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.

REST API: Remove v1 support

3 participants