Conversation
tharvik
left a comment
tharvik
left a comment
There was a problem hiding this comment.
great work on making decentralized work in browser! 🎉
a few logic/safety errors to fix and it can be merged 👍
| this.aggregationResult, | ||
| timeout(undefined, "Timeout waiting on the aggregation result promise to resolve") | ||
| ]) | ||
| } catch (e) { |
There was a problem hiding this comment.
hum, so if this timeouts, we will still procede to the next communication round, but the payload will be invalid (already used by current round and very probably changed by it). I think breaking would be safer.
also, by creating the timeout in there, it is a timeout per communication round, not for the whole onRoundEnd, which is a bit unexcepted.
There was a problem hiding this comment.
Ah yes I agree we should break/return here. However I don't see why a timeout per communication round is unexpected. Shouldn't we timeout whenever we may wait on a promise forever?
There was a problem hiding this comment.
However I don't see why a timeout per communication round is unexpected. Shouldn't we timeout whenever we may wait on a promise forever?
yes, we should timeout for sure, but as each aggregator has a various number of communication round, the "onRoundEnd" timeout varies here which I find a bit weird. you can create the timeout promise at the beginning of the function and reference the same timeout in every communication round.
but yeah, maybe I'm overthinking this (won't be the first time), if you don't find it strange, let's keep it as it is (we don't really have a policy on timeouts anyway).
2 participants
Uh oh!
There was an error while loading. Please reload this page.