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: get subscribers endpoint #1391

Merged
merged 5 commits into from
Feb 26, 2024
Merged

feat: get subscribers endpoint #1391

merged 5 commits into from
Feb 26, 2024

Conversation

chris13524
Copy link
Member

@chris13524 chris13524 commented Feb 6, 2024

Existing /subscribers endpoint downloads all subscribers, which is expensive and takes time. However, usually developers don't need all subscribers, and may just want to know if a specific subscriber is subscribed or not.

This feature is not implemented yet and currently gathering feedback on these docs. Once implemented, this PR will be merged.

@chris13524 chris13524 self-assigned this Feb 6, 2024
Copy link

vercel bot commented Feb 6, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
walletconnect-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 26, 2024 4:33am

@chris13524 chris13524 changed the title feat: get subscribers feat: get subscribers endpoint Feb 6, 2024
- `POST /v1/<project-id>/get-subscribers`
- Each app can call this endpoint 100 times per second with a burst up to 100. Rate limited requests will return a 429 status code.
- `GET /<project-id>/subscribers`
- Each app can call this endpoint 1 time every 5 minutes with a burst up to 2. Rate limited requests will return a 429 status code.
Copy link
Member Author

@chris13524 chris13524 Feb 6, 2024

Choose a reason for hiding this comment

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

Note: rate limit for old endpoint been reduced. I am working with developers to switch to new endpoint before enforcing the new rate limit.

Copy link
Member

Choose a reason for hiding this comment

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

Can we be explicit about what qualifies as a burst here? I'm a bit confused (and suspect external devs may be) how a burst is counted.

Assuming 1 per 5 minutes but we'll allow 2 if within some short interval?

Copy link
Member Author

Choose a reason for hiding this comment

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

This question is asked a lot. I linked to the token bucket Wikipedia article above and tried to summarize it, but not doing a good job

Copy link
Member Author

Choose a reason for hiding this comment

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

Made an issue: #1401

- `GET /subscribers`
- Each app can call this endpoint 1 time per second with a burst up to 5. Rate limited requests will return a 429 status code.
- `POST /v1/<project-id>/get-subscribers`
- Each app can call this endpoint 100 times per second with a burst up to 100. Rate limited requests will return a 429 status code.
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm taking a stab at some numbers for the limits, 100 accounts, 100/s, and 100 burst seem somewhat sensible

Copy link
Member

@bkrem bkrem left a comment

Choose a reason for hiding this comment

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

LGTM 💪 were there any other pieces of metadata beyond the notification types subscribed to that implementing projects asked for?

- `POST /v1/<project-id>/get-subscribers`
- Each app can call this endpoint 100 times per second with a burst up to 100. Rate limited requests will return a 429 status code.
- `GET /<project-id>/subscribers`
- Each app can call this endpoint 1 time every 5 minutes with a burst up to 2. Rate limited requests will return a 429 status code.
Copy link
Member

Choose a reason for hiding this comment

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

Can we be explicit about what qualifies as a burst here? I'm a bit confused (and suspect external devs may be) how a burst is counted.

Assuming 1 per 5 minutes but we'll allow 2 if within some short interval?

@chris13524
Copy link
Member Author

were there any other pieces of metadata beyond the notification types subscribed to that implementing projects asked for?

Nobody has really asked for the notification types yet, but I've had thoughts about this as well as the "created at" timestamp which could've been somewhat helpful with our NFT campaign. Might as well add that too.

@@ -132,8 +215,10 @@ To protect our system and subscribers, various limits and rate limits are in-pla

Rate limits are implemented as [token bucket](https://en.wikipedia.org/wiki/Token_bucket) and contain both rate and burst amounts. On average, a rate of requests can be made. However, since real-world applications often make requests in bursts, this fixed rate can be surpassed temporarily up to the burst amount, provided the app subsequently makes requests below the average in order to recover its bursting capability.

- `POST /notify`:
- `POST /<project-id>/notify`:
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixing this; was inaccurate

Copy link
Contributor

@Cali93 Cali93 left a comment

Choose a reason for hiding this comment

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

LGTM, just a question around the HTTP method

docs/web3inbox/backend-integration.mdx Show resolved Hide resolved
@chris13524 chris13524 merged commit 67e087a into main Feb 26, 2024
2 checks passed
@chris13524 chris13524 deleted the feat/get-subscribers branch February 26, 2024 14:37
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.

5 participants