payment failure cases inabox #1035
Conversation
hopeyen
force-pushed
the
hope/payments-v2-inabox
branch
from
1a5854f to
ea6fe9b
Compare
hopeyen
force-pushed
the
hope/payment-failure-inabox
branch
4 times, most recently
from
89c84de to
d5ce40a
Compare
hopeyen
force-pushed
the
hope/payment-failure-inabox
branch
from
aea2b8f to
9ee1757
Compare
| @@ -0,0 +1,94 @@ | |||
| package integration_test | |||
There was a problem hiding this comment.
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.
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.
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.
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
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