-
Notifications
You must be signed in to change notification settings - Fork 3
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
Split and cleanup client #9
Merged
+742
−450
Merged
Changes from 1 commit
Commits
Show all changes
44 commits
Select commit
Hold shift + click to select a range
42c3806
splitting up client
f321x 2a3b524
merge
f321x b6fc274
separate client logic, cleanup
f321x 3b9d16c
remaining changes
f321x 71efb91
add comments
f321x 29aabb1
put trade mode in metadata
f321x e181c89
minor improvements
f321x 9d6ad91
Create ecash wallet at the parse input phase, the trade parner needs …
rodant c9bb1cf
Fix npubs by usong to_bech32.
rodant b7288cf
rewrite in a more idiomatic form.
rodant 64923f7
Move EscrowClient struct to its module.
rodant 236346c
Inline init_ttrade in main.
rodant 6b10695
Extract wallet creation from RawCliInput
rodant 7e28154
Start introducing the escrow client as state machine for better testa…
rodant 0cf6574
Remove dependency from cli in escrow client.
rodant 7cef1c7
Some clean up.
rodant d32dc48
Move trade mode to the escrow client.
rodant 3ffe54f
Split escrow metadata in the contract and escrow registration parts.
rodant 7e1b21a
Use PublicKey type in trade contract.
rodant 71af4c2
Replace tuple though the registration message struct.
rodant af5121c
Remove utils modules.
rodant 2a88dbc
Remove PubkeyMessage and rearrange models.
rodant 7ee5b40
Fix remaining warnings.
rodant a27ea9a
bump up nostr_sdk version and revert to nip04 direct messages, nip17 …
rodant 85ce4d0
working version with nip17 private direct messages.
rodant a9714b7
Make func sync after review comment.
rodant 13082e7
Introduce timeout and improvements in receive_escrow_message
rodant 542a2be
Better having a get public key method than a get npub.
rodant f001e9c
Remove dependency to cli in ClientNostrClient.
rodant 5014664
Hide nostr client in ClientNostrInstance.
rodant 9ae1ba4
Improve the coordinator loop for running forever.
rodant 0d7a2a2
Improvement for error situations.
rodant 645158c
Bumb up nostr_sdk version.
rodant 5e20036
Handle some error cases.
rodant 6a0e061
Avoid unneeded async func.
rodant 5aaa656
Remove nostr instance and clean up escrow client.
rodant 3bcfde4
Fix race condition in receiving registration and introduce escrow cli…
rodant d038b12
remove unused imports.
rodant 8d79e1c
Deposit enough funds and proceed to a delivery.
rodant 09396a7
Fix typos.
rodant f4b75cb
Update coordinator/src/escrow_coordinator/mod.rs
rodant e2e4382
Initialize the notifications receiver at creation and pull events on …
rodant e1d295e
Define nostr client as field of the escrow client and pass all its fi…
rodant 94d2424
Handle receice errors from a subscription.
rodant File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Fix race condition in receiving registration and introduce escrow cli…
…ent states.
- Loading branch information
commit 3bcfde413584eba8201329fdf199921afe664f4e
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 don't understand why we need to spawn a new task to listen to the escrow msg and then await it afterwards. Couldn't we just send the contract message to the coordinator and then start listening to answers (receive_escrow_message) of the coordinator with a limit higher than 0 (e.g. 1) so we receive "old" messages? I guess there is a slight time benefit if we're already subscribed when expecting the coordinator msg but is it worth this additional complexity?
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.
According to the maintainer of rust-nostr limit(0) is the way to go in case of GiftWrapped event and thus also for NIP-17. See the thread rust-nostr/nostr#173 (comment). Those events have a random created_at field which can be within 2 days in the past. If the client isn't listening before sending the contract, in some situations it misses the answer of the coordinator. I have observed this several times.
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.
When you observed the client missing the events when subscribing after the event was published, did you test with Timestamp::now() or limit(0)? Just for my general understanding, I see now why Timestamp::now() doesn't work due to the random created_at, but in my understanding limit(1) should still work as it just tells the relay to return the last stored event. To make it reliable even in case there are other dms stored to the pubkey that could be returned due to the random created_at we could combine a timestamp - 2days + limit(higher number like 10), then use the newest event by the created_at in the wrapped rumor kind 14 event which should be correct and not random.
Like this for example:
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've observed this issue for a long time, when we had the since(now) filter and also after switching to limit(0). It happened when the buyer sends first the contract and the seller as second one. In this case the coordinator fires almost always the registration before the buyer starts listening for events. You can see this for example on master and in earlier stages of this branch. The fix to this by listening before sending the contract to the coordinator happened way later than the switch to nip-17. Now we still have a similar issue in a later step in case of the seller, it can start listening for the token after the buyer sent the token, I have also observed this. I think the approach with a higher limit isn't reliable enough and we need a different design. The nostr client must start listening from the very beginning and buffer relevant messages, then the escrow client can ask any time for messages with a new version of receive_escrow_message and pick the one it is looking for. It is an idea similar to an actor system where the nostr client acts as a mailbox for the escrow client. Note that in this approach we wouldn't limit the filter, limit(0) would keep listening for new events.
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 think we could just split receiving in two parts. One function that subscribes to the filter and returns the subscription. And another function that pulls events from the subscription (.recv()) channel. So nostr-sdk is the buffer which seems more elegant than spawning tasks.
On the other side, Nostr clients like Amethyst also seem to be able to pull NIP17 events afterwards, i guess they just subscribe to NIP17 events, and then sort them by the created_at in the rumor.
I think we also should consider that traders are most probably not always online at the same time, and go offline between the single steps when waiting for the other party. So we may need to be able to continue at a specific step loading the state from somewhere. Opened issue #14
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 like your idea of trying to use the nostr-sdk as event buffer, I'll try it out.
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 also think Amethyst does a kind of logic as you mentioned.
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 works as you proposed above, subscribing first at start up and then consuming on demand and relying only on the nostr-sdk. Now it is simpler, but I still must clean up a little bit. The escrow client can have a field for the nostr client again :)
Thanks for your comments and ideas, really helpful!
I found also a new problem, some times the escrow client expects a registration but it gets a token message and panics. I could extract this in a new issue, agree? If it doesn't happen (starting first the buyer), it runs successfully so far.
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.
Cool👍
Yeah let's open an issue for this so we get this PR merged and can work on smaller issues in parallel.