feat: expose available needed by chatsdk

[plopezlpz]

Description

Exposing available capacity

Changes

• exposing available capacity for both modes: strict and compensating

How to test

Unit tests included, no further testing

closes #3497

[github-actions]

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:3491

Built from abc3fa8

[Ivansete-status]

LGTM! Thanks for it! 💯

[NagyZoltanPeter]

LGTM, just added some comment to consider.

[NagyZoltanPeter]

Suggested change

	proc getAvailableCapacityRatio*(bucket: TokenBucket, currentTime: Moment): float =
	proc getAvailableCapacityRatio*(bucket: TokenBucket, currentTime = Moment.now()): float =

Might ease the use with defaulting to now() as most of the time that's one in question. WDYT?

[plopezlpz]

yes

[NagyZoltanPeter]

Mind that this version of TokenBucket introduces two modes.
The simple one called strict which behaves as expected, calculates buget use absolute to time period.
The other one can take consideration to refil the bugdet over the capacity based on previous usage.
My guess this calculation will work in case of strict mode, otherwise you can get > 1.0 results.
It's up to you if you would like to manage this case here or not.

[plopezlpz]

true

[fryorcraken]

@plopezlpz what is the intent here?

Is the idea to use this as part of the rate limit manager solution?

In this case, I would suggest that we fully extract the code you need in its own library. cc @NagyZoltanPeter (doesn't have to be straight away, but end goal should be clear).

[plopezlpz]

@plopezlpz what is the intent here?

Is the idea to use this as part of the rate limit manager solution?

In this case, I would suggest that we fully extract the code you need in its own library. cc @NagyZoltanPeter (doesn't have to be straight away, but end goal should be clear).

in the other PR @Ivansete-status suggested using a similar approach as this, since we will need nwaku as dependency I thought of using it directly I just needed this method to calculate the remaining balance. About extracting it, it would be a small one file library, I don't know about it

[Ivansete-status]

This PR is correct and I don't see any harm in exposing the available capacity.
Just adding an additional nitpick comment but I think we can merge that :)

[NagyZoltanPeter]

@plopezlpz what is the intent here?

Is the idea to use this as part of the rate limit manager solution?

In this case, I would suggest that we fully extract the code you need in its own library. cc @NagyZoltanPeter (doesn't have to be straight away, but end goal should be clear).

I agree, when was making DOS protection and found TockenBucket living in nim-libp2p, that would have been the good choice is to extract it into a separate high level utility library. @fryorcraken: Is that what you mean?
Also the reason I added finally into nim-waku code base was the special needs we had back than (and a bug in the original implementation).

[fryorcraken]

I agree, when was making DOS protection and found TockenBucket living in nim-libp2p, that would have been the good choice is to extract it into a separate high level utility library. @fryorcraken: Is that what you mean?

Yes, If we are re-using the same piece of code in now 3 places (nim-libp2p, nwaku and chat sdk) then it would be best to have one separate library that can be imported in all 3 projects.

[Ivansete-status]

Wouldn't it be better to apply the necessary fixes to nim-chronos first, and then only use it?

[fryorcraken]

Wouldn't it be better to apply the necessary fixes to nim-chronos first, and then only use it?

What does "necessary fixes" mean? I assume that ratelimit.nim in chronos has a very specific utility and functionality.
I am not sure it would make sense to change it within chronos and then use it in other application simply because I am not really convinced chronos maintainer would want to see the generalization of their ratelimit.

But this is FOSS, I see 2 valid paths ahead:

1. Do a PR on chronos to generalize the ratelimit and make it fit both nwaku and Chat's rate limit manager use cases. Let's then see if chronos' maintainer welcome the change. I would still find it weird to import this functionality via chronos.

2. Create a generic library that is used by both nwaku and chat's rate limit manager. Youi can then offer to chronos maintainer/open a PR to replace their rate limit manager with ours.

[plopezlpz]

Wouldn't it be better to apply the necessary fixes to nim-chronos first, and then only use it?

What does "necessary fixes" mean? I assume that ratelimit.nim in chronos has a very specific utility and functionality. I am not sure it would make sense to change it within chronos and then use it in other application simply because I am not really convinced chronos maintainer would want to see the generalization of their ratelimit.

But this is FOSS, I see 2 valid paths ahead:

1. Do a PR on chronos to generalize the `ratelimit` and make it fit both nwaku and Chat's rate limit manager use cases. Let's then see if chronos' maintainer welcome the change. I would still find it weird to import this functionality via chronos.

2. Create a generic library that is used by both nwaku and chat's rate limit manager. Youi can then offer to chronos maintainer/open a PR to replace their rate limit manager with ours.

I would go with 2., but 2. would be a really small library, I'll create the repo and see, just a copy/paste

[fryorcraken]

I would go with 2., but 2. would be a really small library, I'll create the repo and see, just a copy/paste

Yes, I came back here for that. What are we talking about here? it's 140 lines of code. I don't see a point in blocking @plopezlpz to try doing some code communism here across 3 different layers of the stack.

Up to you @plopezlpz, my recommendation:

if you can use chronos' out of the box, go for it.
else if you can use nwaku's with minimal changes, do it, and once it works with it within nwaku and some potential dirty exposing, we can extract it
else just rewrite your own.

[arnetheduck]

Do a PR on chronos

a well-argued improvement to chronos would certainly be considered - at the end of the day, a token bucket is a well-described algorithm in literature and if chronos' implementation deviates from that, it's certainly welcome to improve it.

On the other hand, if the implementation here deviates from literature, it's certainly a good question to ask whether it's due to a well-understood reason or if it's a local decision that later might come back to haunt us.