-
Notifications
You must be signed in to change notification settings - Fork 337
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
fix: recover stampissuer on batch creation #2080
Conversation
6c91f9f
to
b37978c
Compare
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.
Great stuff! thank you!
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 28 of 28 files at r1.
Reviewable status: 6 of 28 files reviewed, 2 unresolved discussions (waiting on @acud, @aloknerurkar, @esadakar, and @zelig)
pkg/postage/service.go, line 92 at r1 (raw file):
case errors.Is(err, ErrNotFound): ps.Add(NewStampIssuer( "recovered",
what is this supposed to be?
pkg/postage/service_test.go, line 117 at r1 (raw file):
t.Fatalf("expected no error, got %v", err) } if st.Label() != "recovered" {
label should not be used here
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 6 of 28 files at r1, 22 of 22 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @esadakar and @zelig)
pkg/postage/service.go, line 92 at r1 (raw file):
Previously, zelig (Viktor Trón) wrote…
what is this supposed to be?
an indication that the batch was recovered by the listener
pkg/postage/service_test.go, line 117 at r1 (raw file):
Previously, zelig (Viktor Trón) wrote…
label should not be used here
@zelig I think that using the label here is just fine. Not sure what's the problem of using this field to indicate that the batch was recovered by the listener
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 3 of 22 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @esadakar and @zelig)
Currently, we create a new stampissuer when the user buys new stamps using the
/stamps
API.This makes a call to the blockchain to buy a new postage batch. The operation can take some time and if for some reason the node goes down at this point, we might end up in a state where we have bought the postage batch, but a new stampissuer was not created making this batch not useful.
The change ensures that on batch creation event handled by the listener when we get the update from the blockchain, we will check if a stampissuer is present. And if it's not, we will create it.
This change is![Reviewable](https://camo.githubusercontent.com/23b05f5fb48215c989e92cc44cf6512512d083132bd3daf689867c8d9d386888/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)