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.

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

Feat: tx rate limit #7

Merged
merged 5 commits into from
Jan 17, 2025
Merged

Feat: tx rate limit #7

merged 5 commits into from
Jan 17, 2025

Conversation

dudong2
Copy link

@dudong2 dudong2 commented Jan 14, 2025

ref. a81c3c7


PR checklist

  • Tests written/updated
  • Changelog entry added in .changelog (we use unclog to manage our changelog)
  • Updated relevant documentation (docs/ or spec/) and code comments

@dudong2 dudong2 self-assigned this Jan 14, 2025
@dudong2
Copy link
Author

dudong2 commented Jan 17, 2025

TODO: need to add error to cosmos-sdk after merge.(like ErrMempoolIsFull)

@dudong2 dudong2 requested a review from cloudgray January 17, 2025 06:58
@cloudgray
Copy link

cloudgray commented Jan 17, 2025

TODO: need to add error to cosmos-sdk after merge.(ref. ErrMempoolIsFull)

@dudong2
The error type ErrMempoolIsFull has existed before, and the newly added error seems to be ErrMempoolRateLimitExceeded. Is this error you’re referring to, right?

@dudong2
Copy link
Author

dudong2 commented Jan 17, 2025

Yes, need to register an error in the cosmos-sdk to check that a comet-related error occurred at the broadcast tx level.

Copy link

@cloudgray cloudgray left a comment

Choose a reason for hiding this comment

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

LGTM except for just one trivial comment.

}

func (e ErrMempoolRateLimitExceeded) Error() string {
return fmt.Sprintf("mempool rate limit exceeded: rate %d, count %d", e.Rate, e.Count)

Choose a reason for hiding this comment

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

I think rate: %d is little bit obvious.
How about rate: "%d txs per block?

Copy link
Author

Choose a reason for hiding this comment

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

agree, i changed rate to limit

Copy link

@cloudgray cloudgray left a comment

Choose a reason for hiding this comment

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

LGTM

@dudong2 dudong2 merged commit 782b0da into basechain/develop Jan 17, 2025
14 of 15 checks passed
@dudong2 dudong2 deleted the feat/rate-limit branch January 17, 2025 08:17
dudong2 added a commit that referenced this pull request Feb 18, 2025
* feat: Rate limit

* chore: Change default valuetest: Fix tests

* chore: Change default value to -1

* test: Add mock func for ResetRateLimitCounter

* chore: Rate -> Limit
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants