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

First draft spec rate limiter #4

Merged
merged 20 commits into from
Dec 22, 2022
Merged

First draft spec rate limiter #4

merged 20 commits into from
Dec 22, 2022

Conversation

angbrav
Copy link
Contributor

@angbrav angbrav commented Dec 16, 2022 •
edited by adizere
Loading

Rendered

Transforms this informal doc into a bit more formal spec.

  • Includes pseudocode
  • Some text descriptions

It would be nice to adapt the spec to implement the Middleware interface specified in ICS 30. AS: Update: I captured this as a known limitation inside the spec.

@angbrav angbrav marked this pull request as draft December 16, 2022 15:15
@adizere adizere marked this pull request as ready for review December 19, 2022 15:16
adizere
adizere approved these changes Dec 20, 2022
View reviewed changes
Copy link
Member

@adizere adizere left a comment

Choose a reason for hiding this comment

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

Major changes I did: fixed todos, added a figure to describe the design, and added a ## Limitations and Recommendations subsection.

Will let @angbrav and @ancazamfir sign off and merge this.

ancazamfir
ancazamfir requested changes Dec 20, 2022
View reviewed changes
Copy link
Contributor

@ancazamfir ancazamfir left a comment

Choose a reason for hiding this comment

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

Some initial comments after first pass. I need to spend more time on this, especially on the pseudocode.

security/rate-limiter.md Outdated Show resolved Hide resolved
security/rate-limiter.md Outdated Show resolved Hide resolved
security/rate-limiter.md Outdated Show resolved Hide resolved
security/rate-limiter.md Outdated Show resolved Hide resolved
security/rate-limiter.md Outdated Show resolved Hide resolved
security/rate-limiter.md Outdated Show resolved Hide resolved
security/rate-limiter.md Outdated Show resolved Hide resolved
security/rate-limiter.md Outdated Show resolved Hide resolved
security/rate-limiter.md Outdated Show resolved Hide resolved
security/rate-limiter.md Outdated Show resolved Hide resolved
angbrav
angbrav commented Dec 20, 2022
View reviewed changes
Copy link
Contributor Author

@angbrav angbrav left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@ancazamfir thanks for fixing the pseudocode. Looks good to me

security/rate-limiter.md Outdated Show resolved Hide resolved
angbrav
angbrav commented Dec 20, 2022
View reviewed changes
security/rate-limiter.md Outdated Show resolved Hide resolved
@ancazamfir
Copy link
Contributor

Just noticed the two figures have different types (.svg vs .png) and also the fonts are different. We should fix this.

angbrav
angbrav commented Dec 21, 2022
View reviewed changes
security/rate-limiter.md Outdated Show resolved Hide resolved
@angbrav
Copy link
Contributor Author

angbrav commented Dec 21, 2022

I did a full pass. Main changes (please review):

  • add definitions of native token and source chain
  • make FlowPath a triplet: this includes going through the text, adapting the pseudocode, etc
  • say in the limitations section that the functions for setting, changing and removing ratelimiters are not specified

I have also left a comment w.r.t. to a question that I have with the sendPacket pseudocode. Other than that, I am fine with the version.

@angbrav
Copy link
Contributor Author

angbrav commented Dec 21, 2022

Just noticed the two figures have different types (.svg vs .png) and also the fonts are different. We should fix this.

It would be nice to fix, but should not be a blocker to getting this merge I believe.

@ancazamfir
Copy link
Contributor

Just noticed the two figures have different types (.svg vs .png) and also the fonts are different. We should fix this.

It would be nice to fix, but should not be a blocker to getting this merge I believe.

I made the change as it was very simple.

ancazamfir
ancazamfir approved these changes Dec 21, 2022
View reviewed changes
Copy link
Contributor

@ancazamfir ancazamfir left a comment

Choose a reason for hiding this comment

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

I think it's good to merge! Amazing work @angbrav and @adizere!

@angbrav
Copy link
Contributor Author

angbrav commented Dec 22, 2022

Merging! great work!

@angbrav angbrav merged commit 952b453 into main Dec 22, 2022
@adizere adizere deleted the manuel/rate-limiter-draft branch January 3, 2023 11:00
@adizere adizere mentioned this pull request Jan 9, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ancazamfir ancazamfir ancazamfir approved these changes

@adizere adizere adizere approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@angbrav @ancazamfir @adizere