feat: reconnection in nip47 #72
Conversation
internal/nostr/models.go
Outdated
| Since time.Time | ||
| Until time.Time | ||
| Limit int | ||
| Search string | ||
| CreatedAt time.Time | ||
| UpdatedAt time.Time | ||
| Uuid string `gorm:"type:uuid;default:gen_random_uuid()"` | ||
| Uuid string `gorm:"type:uuid;default:gen_random_uuid()"` | ||
| EventChan chan *nostr.Event `gorm:"-"` |
There was a problem hiding this comment.
I think it would be good to split the DB model from the application model. Then we do not need these gorm hints
| REQUEST_EVENT_PUBLISH_CONFIRMED = "confirmed" | ||
| REQUEST_EVENT_PUBLISH_FAILED = "failed" | ||
| REQUEST_EVENT_PUBLISH_CONFIRMED = "CONFIRMED" | ||
| REQUEST_EVENT_PUBLISH_FAILED = "FAILED" |
There was a problem hiding this comment.
are these still used and is it ok to change the casing? will the previous values be manually migrated?
There was a problem hiding this comment.
Yup, we can do it this way instead of writing a migration
UPDATE request_events
SET state = UPPER(state);
There was a problem hiding this comment.
@im-adithya keep in mind for an open source project we typically also have to think about other people who might run this.
it's fine here, but keep this in mind.
internal/nostr/nostr.go
Outdated
| } | ||
| svc.postEventToWebhook(event, requestData.WebhookUrl) | ||
| }() | ||
| go svc.startSubscription(svc.Ctx, &subscription) |
There was a problem hiding this comment.
@bumi mentioned:
I might think a bit too complicated there and maybe the two options (webhook + sync) is not needed
But aren't they both separate, and good usecases? otherwise you are forced to use a webhook?
There was a problem hiding this comment.
both are good and important use cases. not sure if we should continue to have it in one endpoint though if it's too complicated.
There was a problem hiding this comment.
Now that we simplified code, it's actually easier to support both in a single endpoint...
| svc.db.Save(subscription) | ||
| svc.db.Save(requestEvent) | ||
| }() | ||
| relay, isCustomRelay, err := svc.getRelayConnection(ctx, subscription.RelayUrl) |
There was a problem hiding this comment.
Marking here: the isCustomRelay logic looks quite dangerous, it disconnects the relay after each request and in multiple places. If there are two simultaneous requests to the same custom relay, this could break
internal/nostr/nostr.go
Outdated
| requestEvent := RequestEvent{} | ||
| findRequestResult := svc.db.Where("nostr_id = ?", requestData.SignedEvent.ID).Find(&requestEvent) | ||
|
|
||
| if findRequestResult.RowsAffected != 0 { |
There was a problem hiding this comment.
This code all looks duplicated from another method - can you try DRY it up?
|
Right now we are only storing the subscription in the DB to get an ID from there which we then use to store subscriptions in the |
Closes #66
processEvents(conditionally for NIP47 requests)