Remove AppRequestId from api_types,#6969
Conversation
as shown by the diffs was redundant
AgeManning
left a comment
There was a problem hiding this comment.
Yeah nice.
Looking at this, I think the main logic change is around the Status message. It looks like this PR now sends RPC errors related to status message to Sync.
We probably should avoid doing that, but it might re-introduce this concept of Router vs Sync in case we want to leave it generic if we add more non-sync rpc things later.
Not sure, curious about what you think
|
Yeah. I think these changes look sane. Just to be clear tho. We are now assuming every RPC call is related to sync and no longer have a way to distinguish non-sync network messages. I think in the current state it's fine, because Lighthouse isn't really making non-sync messages. Although, it looks like there was a panic if we ran this, in the previous version, so it might be worth quickly running this PR to make sure it works as expected. An easy way to do it locally is here: https://github.com/sigp/lighthouse/tree/stable/testing/network_testing |
|
Closing in favour of #7238 |
It seems we are never matching on it to react on it but rather to log
crit!as those situations should not exist first hand.Statusrequest handling doesnt seem to care if it'sRouterorSync.Therefore it seems redundant to have it, but please give it a check as I may have missed something