algod: Migrate internal uses of v1 algod API to v2#4684
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4684 +/- ##
==========================================
- Coverage 54.49% 54.43% -0.07%
==========================================
Files 407 407
Lines 52425 52499 +74
==========================================
+ Hits 28569 28576 +7
- Misses 21472 21540 +68
+ Partials 2384 2383 -1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
…ges line up with v2 response
* Delete extraneous function makeTestBlock inline * Add test for blockstats event details * Delete another comment
Co-authored-by: Will Winder <wwinder.unh@gmail.com>
|
I looked at |
|
@brianolson Thanks a lot for confirming the pingpong files 👍
Yes, we can do that - I noted down things we can delete in a near future PR (deleting all deprecated libgoal/client code, tests, deprecated goal commands) to keep diffs smaller for reviewers and narrow the scope of work here. |
michaeldiamant
left a comment
michaeldiamant
left a comment
There was a problem hiding this comment.
@algochoi Thanks for covering a lot of ground here - Looks ready to go from my side.
| if err == nil { | ||
| var b rpcs.EncodedBlockCert | ||
| err = protocol.DecodeReflect(resp, &b) | ||
| err = protocol.DecodeReflect(resp, &blockCert) |
There was a problem hiding this comment.
nit: I know you didn't write the original code, but rpcs.EncodedBlockCert has generated msgp methods, so it would benefit slightly from using protocol.Decode instead of protocol.DecodeReflect
| } | ||
|
|
||
| reserve, err := client.AccountInformation(params.ReserveAddr) | ||
| reserve, err := client.AccountInformationV2(*asset.Params.Reserve, true) |
There was a problem hiding this comment.
I think client.AccountAssetInformation(*asset.Params.Reserve, assetID) could be used instead of client.AccountInformationV2
There was a problem hiding this comment.
Followed up offline: It looks like we'd want to change the behavior so that goal asset info fails if the reserve doesn’t hold the asset - this will likely involve changing a test (test/scripts/e2e_subs/asset-misc.sh) to address this in a follow-up PR.
| } else { | ||
| // Fetch the current state, to fill in as a template | ||
| current, err := c.AccountInformation(creator) | ||
| current, err := c.AccountInformationV2(creator, true) |
There was a problem hiding this comment.
I was going to suggest this could be improved with client.AccountAssetInformation, but I don't think this side of the if statement needs to exist at all.
There was a problem hiding this comment.
I think I agree - I'll follow up on this in another PR
|
Merging - Have agreed with @algochoi that the remaining open threads will be addressed in a prior PR before closing the story. Merging the PR to minimize merge conflicts. |
5 participants
Summary
As part of sunsetting v1 algod APIs, we want to only use v2 algod APIs in the internal REST client. Relates to #2976.
This PR changes the client code and internal tests/tooling to use v2 models and endpoints.
There are two v1 endpoints that do not have an equivalent endpoint in v2:
/v1/account/%s/transactionsrestClient_test.gothat uses this endpoint - resolve/delete test when v1 API is sunsetted./v1/account/%s/transactions/%s/v2/account/%s/transactions/pendingMajor Changes:
restClient.go,libgoal: Add v2 endpoints and use v2 models/responses. Add helper function inlibgoalto parse pending transaction responses into a struct by decoding from msgpack.algoh: Use v2 Block instead of v1 Block. Since the v2BlockAPI does not return the Proposer information, we get the raw block and parse it for the block + certificate.loadgenerator(along with tests) now uses the v2 endpoint to send transactionspingpongnow uses the v2 models and endpointsassetinformation with v2 responses - they now have pointer fields to denote whether a field is empty or notTesting:
masterbranch is <5%loadgenerator!Future work
For the sake of keeping diffs small in a single PR, we should do the following in a future ticket:
legacyListParticipationKeysCommand)