-
Notifications
You must be signed in to change notification settings - Fork 17
Conversation
In ilp-protocol-stream, `Stream`s emit `"outgoing_money"` events before `connection.totalDelivered` is updated. This resulted in the extension ignoring the first `outgoing_money` event, since it mistakenly assumed that no money was delivered. In my tests, this shortened the time-to-`monetizationstart` from 2500-2800ms down to 500-700ms.
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 find!
Is this speedup also after applying optimizations like exchange rate and packet size hints or have those not gone in yet? I would be curious if they can get the time down even further |
It's without, I think. I tested enabling them and they don't make as much of a different as I expected, so I'm actually looking into that next. |
Is this fix applicable to the oauth script also? right now the two have different modules that send on streams |
I didn't think of that. Yes, I think it is applicable, I'll update the PR. |
@@ -331,7 +331,8 @@ class StreamAttempt { | |||
|
|||
return new Promise((resolve, reject) => { | |||
const onMoney = (sentAmount: string) => { | |||
this.onMoney(sentAmount) | |||
// Wait until `setImmediate` so that `connection.totalDelivered` has been updated. |
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.
Nice work Mr @sentientwaffle !
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.
Maybe ilp-protocol-stream should be updated to set that beforehand ?
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.
Since updating the hold may not change the amount that isn't desirable.
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.
Can you elaborate on that?
Note that I'm not a STREAM/ILP expert.
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.
You're right, stream could set the amount beforehand, but I think it would qualify as a breaking change (albeit just barely). I've made a note-to-self to include that change the next time I make a breaking stream change. I don't think it warrants one on its own since it's so simple to work around.
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, so outgoing_money is when money is actually delivered right? Or am I wrong there?
It's definitely a change but one could reasonably argue it's actually a bug and the current behavior is incorrect. Would it really actually "break" any client code or simply give more correct results ?
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, outgoing_money
is the notification that money has been successfully delivered on a stream.
@sentientwaffle |
@sentientwaffle |
@sublimator Testing against a local server may not work because the packets would arrive so quickly that the delay may be too small to notice. I was testing against coil's staging server. |
actually, the delay was noticeable on local server if that means anything
to you!?
will PM you to discuss soon
|
In ilp-protocol-stream,
Stream
s emit"outgoing_money"
events beforeconnection.totalDelivered
is updated. This resulted in the extension ignoring the firstoutgoing_money
event, since it mistakenly assumed that no money was delivered.In my tests, this shortened the time-to-
monetizationstart
from 2500-2800ms down to 500-700ms.