-
Notifications
You must be signed in to change notification settings - Fork 31
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
chore: update go-nostr to v0.25.5 #165
Conversation
main.go
Outdated
sub, err := relay.Subscribe(ctx, svc.createFilters()) | ||
if err != nil { | ||
//err being non-nil means that we have an error on the websocket error channel. In this case we just try to reconnect. | ||
svc.Logger.WithError(err).Error("Got an error from the relay while subscribing. Reconnecting...") |
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.
An immediate,single reconnection attempt doesn't seem quite useful. Just Fatal
immediately or do some backoff (I would do the first).
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.
Ok, thanks!
service.go
Outdated
nostrEvent := NostrEvent{} | ||
result := svc.db.Where("nostr_id = ?", event.ID).First(&nostrEvent) | ||
if result.Error != nil { | ||
svc.Logger.WithFields(logrus.Fields{ | ||
"eventId": event.ID, | ||
"status": status, | ||
"replyEventId": resp.ID, | ||
"publishErr": publishErr, |
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.
Why not check publishErr immediately instead of doing another db operation?
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, that sounds better. I will update it
service.go
Outdated
nostrEvent.State = NOSTR_EVENT_STATE_PUBLISH_CONFIRMED | ||
nostrEvent.RepliedAt = time.Now() | ||
|
||
if status == nostr.PublishStatusFailed || publishErr != nil { |
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 wouldn't switch around the cases here without a good reason?
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.
now I also check if the error is not nil. I think the error should be checked before proceeding. Maybe there is a better way?
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.
Edit: I saw your other comment.
@@ -218,7 +221,7 @@ func (svc *Service) createResponse(initialEvent *nostr.Event, content interface{ | |||
} | |||
resp := &nostr.Event{ | |||
PubKey: svc.cfg.IdentityPubkey, | |||
CreatedAt: time.Now(), | |||
CreatedAt: nostr.Now(), |
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.
what is nostr.Now() ? 🤔
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.
@kiwiidb I addressed your feedback. Could you look again? |
Fixes #162
First part of updating dependencies.
A few things changed such as notice and error handling in StartSubscription.