-
Notifications
You must be signed in to change notification settings - Fork 198
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
payment failure cases inabox #1035
Conversation
1a5854f
to
ea6fe9b
Compare
89c84de
to
d5ce40a
Compare
aea2b8f
to
9ee1757
Compare
@@ -0,0 +1,94 @@ | |||
package integration_test |
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.
I wonder if these tests could be moved to unit tests? like in accountant_test.go
?
Also, should we also test payment failure cases from disperser server if there isn't already? (to cover the case that user isn't running accountatnt)
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.
hmm I made these tests for the interaction of disperser client and the server, and shows when a blob should be dispersed and when it should fail with respect to payments. I don't think they could be moved to unit test.
Disperser client would build accountant if it wasn't provided with one, so alternatively we could add rogue accountant implementations that makes invalid payment headers
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.
hmm I made these tests for the interaction of disperser client and the server
Is the server logic relevant in these tests? It looks like they mostly test the accountant logic (server could have the payment metering disabled and allowed all requests and these tests would still pass)
Disperser client would build accountant if it wasn't provided with one
Yes, but not all requests may originate from the disperser client shipped in this repo. Anyone is free to disperse blobs without running accountant, in which case, API server should enforce correct metering scheme. I was referring to payment failure cases from the API server payment metering, not from the accountant.
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.
Is the server logic relevant in these tests? It looks like they mostly test the accountant logic (server could have the payment metering disabled and allowed all requests and these tests would still pass)
Hmm I understood your concerns. I updated the tests to override the accountant with erroneous payment state so that accountant doesn't account correctly, and focus on testing failures from API server logic. (if payment is disabled, the tests should not pass as the expected failure would instead be nil)
not all requests may originate from the disperser client shipped in this repo. Anyone is free to disperse blobs without running accountant, in which case, API server should enforce correct metering scheme.
by this logic, should we have a set of tests that doesn't use DisperserClient? The existing tests all utilize NewDisperserClient
to make API requests
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.
Yes, I think tests for accountant and metering from API server should be independently done covering potentially identical/similar scenarios so that they're covered in both client & server side. If we do this, we could probably have them as unit tests in accountant_test.go
and apiserver/server_v2_test.go
instead of as part of the inabox test
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.
There are existing unit tests for both meterer and accountants, covering similar logic on both sides. What's done here is the interaction between accountant and meterer. Do you think it is not necessary to test the interaction? I'm okay to close the PR if that's the case?
Why are these changes needed?
explicitly running out of funds for both payments (will rebase after merging #991, #1033, and #1034 , don't need to review yet)
Checks