Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: rendezvous refactor #1183

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
+91 −35
Open

feat: rendezvous refactor #1183

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d643f95
rendezvous refactor
SionoiS Aug 28, 2024
3b8d520
added back consts & renaming
SionoiS Aug 30, 2024
c6e79d6
Merge branch 'master' into feat--rendezvous-refactor
lchenut Sep 19, 2024
771da4e
added ttl limits & better error msg
SionoiS Sep 19, 2024
8aa5c43
tests & raising exceptions
SionoiS Sep 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion libp2p/protocols/rendezvous.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -515,7 +515,7 @@ proc batchAdvertise*(
raise newException(RendezVousError, "Invalid namespace")

if ttl notin rdv.minDuration .. rdv.maxDuration:
raise newException(RendezVousError, "Invalid time to live")
raise newException(RendezVousError, "Invalid time to live: " & $ttl)

let sprBuff = rdv.switch.peerInfo.signedPeerRecord.encode().valueOr:
raise newException(RendezVousError, "Wrong Signed Peer Record")
Expand Down Expand Up @@ -690,6 +690,15 @@ proc new*(
minDuration = MinimumDuration,
maxDuration = MaximumDuration,
): T =
if minDuration < 1.minutes:
raiseAssert("TTL too short: 1 minute minimum")
Copy link
Collaborator

@diegomrsantos diegomrsantos Sep 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure raising a Defect here is appropriate. Defect types (like those raised by raiseAssert) are meant to indicate programming errors - situations that should never occur if the code is correct. They are not intended for handling invalid input or runtime errors that can occur during normal operation. The invalid minDuration and maxDuration values represent incorrect usage of the new procedure by the caller, possibly due to user input or misconfiguration. These are not programming errors but runtime issues that should be handled gracefully. By raising an Exception, you allow the caller of the procedure to catch and handle the exception appropriately.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, you're right, raiseAssert is not the good call here... I did my last review a bit too fast (or too early).


if maxDuration > 72.hours:
raiseAssert("TTL too long: 72 hours maximum")

if minDuration >= maxDuration:
raiseAssert("Minimum TTL longer than maximum")

let
minTTL = minDuration.seconds.uint64
maxTTL = maxDuration.seconds.uint64
Expand Down
Loading