update error handling #809
Conversation
eth/p2p/discoveryv5/protocol.nim
Outdated
| # The handling of the message needs to be done after adding the ENR. | ||
| d.handleMessage(packet.srcIdHs, a, packet.message, packet.node) | ||
| else: | ||
| if decoded.isErr: |
There was a problem hiding this comment.
If we are to restructure this without the big if/else clause (which is indeed cleaner) then it would be even cleaner to use Result's valueOr.
eth/p2p/discoveryv5/protocol.nim
Outdated
| proto.receive(Address(ip: raddr.toIpAddress(), port: raddr.port), buf) | ||
| let res = proto.receive(Address(ip: raddr.toIpAddress(), port: raddr.port), buf) | ||
| if res.isErr: | ||
| debug "Failed to process received packet", error = res.error |
There was a problem hiding this comment.
It is rather redundant to forward the error message to be logged here again, when it already was up (however trace and thus also altering here the log level).
It is also not fully clear what failed processing should include. I would perhaps also consider the unsolicited whoareyou message an processing error. While here it is rather meant for failed decoding of the packet (and the decoding error gets lost in the passed along Result).
There was a problem hiding this comment.
Oh right, we can just discard its result then ? As if the functions returns some error , it would have been logeed already.
There was a problem hiding this comment.
I meant that you can just remove the DiscResult[void] as return value in the receive call in the first place.
There was a problem hiding this comment.
ok, that's fine then.
FYI, you can update your nimbus-eth1 PR (in draft) to use this commit of nim-eth. That way it is easier to see why certain changes are made or how the API should look.
eth/p2p/discoveryv5/protocol.nim
Outdated
| responseTimeout: config.responseTimeout, | ||
| rng: rng) | ||
|
|
||
| proc newDiscoveryV5*( |
There was a problem hiding this comment.
As we are creating new API here to initialize the discovery protocol I would prefer to go for the func new(T: type Protocol, ... way of doing this (see also https://status-im.github.io/nim-style-guide/language.objconstr.html ).
We can perhaps alias Protocol to DiscoveryV5Protocol to make this more clear (the original Protocol type naming is rather unfortunate).
There was a problem hiding this comment.
We also need to have the same parameters to be passed along as exist for newProtocol
eth/p2p/discoveryv5/protocol.nim
Outdated
| ) | ||
|
|
||
| protocol.transp = transp | ||
| protocol.seedTable() |
There was a problem hiding this comment.
I think it will become rather confusing for the user considering there are two different types of initialization, one that requires open and one that does not, and seeds the table already in the init. Easy "mistake" for a user would be to call open also after doing newDiscoveryV5WithTransport.
As a possible solution for this we could move everything out of open and deprecate it. However that would mean the TransportOsError moves to the init.
Another possibility is to move the seedTable to the start proc, and for the open call, make it a null operation in case the transport is already open.
There was a problem hiding this comment.
@kdeme as I add open close functions according to shared transport as well, then I'll remove the seedTable from the new functions
|
So the issue I have with this version is that you can still use the open/close functions no matter which new call you used. The API relies heavily on the naming for the users to use it correctly. I prefer not do rely on naming. |
|
@kdeme can you please suggest some solution ? Either we can alias it with a different typename or something else. |
|
@kdeme I tried updating the submodules at nim-eth1 on my branch , I don't know why it hangs at cloning eth-tests submodule updates . I updated gitmodules nim-eth url and branch in my branch and was trying to update it with the new |
|
Why not use inheritance? type
DiscoveryV5Base = ref object of RootRef
...
common fields
DiscoveryV5Ref = ref object of DiscoveryV5Base
fields
DiscoveryV5WithTransportRef = ref object of DiscoveryV5Base
fields
# all shared procs will use `DiscoveryV5Base` as their first param
# Specific to DiscoveryV5Ref
proc open(p: DiscoveryV5Ref) =
....
|
This is a workaround to fix #3573. A better fix is to wait for status-im/nim-eth#809.
This is a workaround to fix #3573. A better fix is to wait for status-im/nim-eth#809.
Another option could be to just have a |
3 participants
Question:
Should I add open, close functions for shared port ? @kdeme