-
Notifications
You must be signed in to change notification settings - Fork 339
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
feat(debugapi, postage): dilute batch handling #2410
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very nice. I have some small requests
Reviewed 16 of 16 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @aloknerurkar, @notanatol, and @zelig)
openapi/SwarmDebug.yaml, line 924 at r1 (raw file):
$ref: "SwarmCommon.yaml#/components/schemas/BatchID" required: true description: Swarm address of the stamp
batch ID to dilute
pkg/debugapi/postage.go, line 385 at r1 (raw file):
Previously, aloknerurkar wrote…
This is how we do it in other API endpoints as well. The reasoning behind it, AFAIK:
Empty string check
x == ""
is far more efficient thanlen()
check. If in the||
the first condition is true, the second one is not evaluated at all. This is why I would prefer having this check and evaluating empty strings slightly early.
if x == ""
the router will not hit the endpoint and return an error since this parameter is mandatory. so idStr
will never be ""
. If this is on other endpoints then this is a mistake that needs to be rectified
pkg/debugapi/postage_test.go, line 115 at r1 (raw file):
t.Run("with too many requests error", func(t *testing.T) { wait, done := make(chan struct{}), make(chan struct{})
since you're testing the middleware here it might be useful to just have one test for that (since this test is copied between different endpoints) maybe it would be easier to just pull this test out and run it on both endpoints, instead of copying it
pkg/debugapi/postage_test.go, line 485 at r1 (raw file):
t.Run("with too many requests error", func(t *testing.T) { wait, done := make(chan struct{}), make(chan struct{}) contract := contractMock.New(
see ^^
pkg/debugapi/postage_test.go, line 655 at r1 (raw file):
t.Run("with too many requests error", func(t *testing.T) { wait, done := make(chan struct{}), make(chan struct{}) contract := contractMock.New(
see ^^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 16 of 16 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @aloknerurkar and @notanatol)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dismissed @acud and @notanatol from 5 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @aloknerurkar)
openapi/SwarmDebug.yaml, line 924 at r1 (raw file):
Previously, acud (acud) wrote…
batch ID to dilute
Done.
pkg/debugapi/postage.go, line 385 at r1 (raw file):
Previously, acud (acud) wrote…
if
x == ""
the router will not hit the endpoint and return an error since this parameter is mandatory. soidStr
will never be""
. If this is on other endpoints then this is a mistake that needs to be rectified
Done.
pkg/debugapi/postage_test.go, line 115 at r1 (raw file):
Previously, acud (acud) wrote…
since you're testing the middleware here it might be useful to just have one test for that (since this test is copied between different endpoints) maybe it would be easier to just pull this test out and run it on both endpoints, instead of copying it
Done.
pkg/debugapi/postage_test.go, line 485 at r1 (raw file):
Previously, acud (acud) wrote…
see ^^
Done.
pkg/debugapi/postage_test.go, line 655 at r1 (raw file):
Previously, acud (acud) wrote…
see ^^
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @aloknerurkar)
This change is