Skip to content
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

tools: new endpoints for block generator #5257

Merged
merged 17 commits into from
Apr 12, 2023

Conversation

shiqizng
Copy link
Contributor

@shiqizng shiqizng commented Apr 5, 2023

this PR adds new endpoints /v2/ledger/sync/ and /v2/deltas/{round} to block generator.


func accountToIndex(a basics.Address) (addr uint64) {
// Make sure we don't generate a zero address by adding 1 to i
return binary.LittleEndian.Uint64(a[:]) - 1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved to utils.

@codecov
Copy link

codecov bot commented Apr 5, 2023

Codecov Report

Merging #5257 (4c1830f) into master (f8a130e) will increase coverage by 0.00%.
The diff coverage is 30.43%.

❗ Current head 4c1830f differs from pull request most recent head 5681dab. Consider uploading reports for the commit 5681dab to get more accurate results

@@           Coverage Diff           @@
##           master    #5257   +/-   ##
=======================================
  Coverage   53.75%   53.76%           
=======================================
  Files         450      450           
  Lines       56191    56236   +45     
=======================================
+ Hits        30207    30235   +28     
- Misses      23632    23656   +24     
+ Partials     2352     2345    -7     
Impacted Files Coverage Δ
tools/block-generator/generator/server.go 7.04% <0.00%> (-19.12%) ⬇️
tools/block-generator/generator/generate.go 58.42% <48.27%> (-1.19%) ⬇️
tools/block-generator/generator/utils.go 75.00% <53.84%> (-25.00%) ⬇️

... and 8 files with indirect coverage changes

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

Copy link
Contributor

@tzaffi tzaffi left a comment

Choose a reason for hiding this comment

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

Still reading through the PR, but asking a couple of questions down for now.

tools/block-generator/generator/server.go Outdated Show resolved Hide resolved
winder
winder previously approved these changes Apr 7, 2023
@shiqizng shiqizng changed the title [WIP] tools: new endpoints for block generator tools: new endpoints for block generator Apr 10, 2023
@shiqizng shiqizng marked this pull request as ready for review April 10, 2023 16:26
@shiqizng
Copy link
Contributor Author

@cce how could we update the block generator interface so it could work for pingpong?

@shiqizng shiqizng requested review from tzaffi and winder April 10, 2023 16:55
tzaffi
tzaffi previously approved these changes Apr 10, 2023
Copy link
Contributor

@tzaffi tzaffi left a comment

Choose a reason for hiding this comment

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

LGTM - but leaving an ignorable nit/qn

func parseAccount(path string) (string, error) {
if !strings.HasPrefix(path, accountsQueryPrefix) {
return "", fmt.Errorf("not a accounts query: %s", path)
func parseURL(path string) (string, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

generalizing the parser for all endpoints. I didn't think it was necessary to check the path prefix since each handler function is associated with only one endpoint.

Eric-Warehime
Eric-Warehime previously approved these changes Apr 11, 2023
Copy link
Contributor

@tzaffi tzaffi 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 - just recommend reviving the spirit of a no longer obsolete unit test

@shiqizng
Copy link
Contributor Author

Looks Good - just recommend reviving the spirit of a no longer obsolete unit test

tests updated.

Copy link
Contributor

@tzaffi tzaffi left a comment

Choose a reason for hiding this comment

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

Approving, but leaving a suggested renaming of a test-only struct field that can be ignored.

var testcases = []struct {
name string
url string
expectedRound uint64
expectedRound string
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
expectedRound string
expectedParam string

Copy link
Contributor

Choose a reason for hiding this comment

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

Or maybe expectedPart ? No big deal regardless.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll update this in the next PR.

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.

4 participants