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

Bug fix: Limiting Number of Sponsorship Calls for New Users #52

Closed
wants to merge 12 commits into from

Conversation

lykimq
Copy link
Collaborator

@lykimq lykimq commented Jan 26, 2024

Git Issue #45: Limiting Number of Sponsorship Calls for New Users

Description:
Currently, there exists a manual limitation on the number of calls for a specific sponsee (giving their address and so on).

However, the correct condition should be to set a maximum limit on a number of calls from any address or new user, without having to manually enter them one by one. This ensures that only the first N calls are sponsored for each user.

  • Remove the current sponsee_address in condition
  • Add the option to choose the contract to set a condition
  • Add a condition for N new users (tie to the chosen contract), currently the condition for the users is the last 4 newest users registered in the database.

src/crud.py Outdated Show resolved Hide resolved
@lykimq lykimq marked this pull request as ready for review January 30, 2024 13:29
@lykimq lykimq force-pushed the quyen@max_sponsee branch 2 times, most recently from c53ec0f to fd247d2 Compare January 30, 2024 13:39
Copy link
Collaborator

@aguillon aguillon left a comment

Choose a reason for hiding this comment

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

I haven't tried due to mysterious, but hopefully temporary, technical reasons. However, there's no way this code works. Can you please describe how you tested it?

src/crud.py Outdated Show resolved Hide resolved
src/crud.py Outdated Show resolved Hide resolved
src/models.py Outdated Show resolved Hide resolved
src/routes.py Outdated Show resolved Hide resolved
src/routes.py Outdated Show resolved Hide resolved
src/schemas.py Outdated Show resolved Hide resolved
src/schemas.py Outdated Show resolved Hide resolved
@lykimq lykimq force-pushed the quyen@max_sponsee branch 2 times, most recently from d87a0d7 to 30b4742 Compare January 30, 2024 14:24
@lykimq lykimq force-pushed the quyen@max_sponsee branch 2 times, most recently from 9e0df0e to 2cc8de0 Compare February 2, 2024 11:33
Copy link
Collaborator

@aguillon aguillon left a comment

Choose a reason for hiding this comment

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

This does not seem correct at all, for various reasons.

sponsee_address: str
class MaxCallsPerContractForNewUsersCondition(ConditionBase):
contract_id: UUID4
user_id: UUID4
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you add this?

recent_users: List[models.User] = (
db.query(models.User)
.order_by(models.User.created_at.desc())
.limit(4)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What? Why such an arbitrary limit?

if not entrypoint.is_enabled:
raise EntrypointDisabled()

# Retrieving the user_id
Copy link
Collaborator

Choose a reason for hiding this comment

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

In this context, it's not clear who you call "the user". According to what you write just after, it seems to be the user who added the contract; however, you're also testing if the user is "new" (for some definition of "new", but whatever), which, I suspect, is your way of translating the real-world example I gave you. In this example, I was giving you the idea that we needed to change the "max calls per user" condition so that it can be used to limit the activity of some users without impacting newer users, but your translation of this requirement seems rather fanciful.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The general idea is that, in the context of this issue/PR, a user should not be considered "new" because of the age of their account, but rather because of their recent activity relatively to the sponsored contract.

However, when we talk about "user" in this context, we're definitely talking about end-users, or sponsees (users whose transactions are paid for by the API) and not contracts' owners.

user = crud.get_user_by_address(db, owner_address)
user_id = user.id
if not crud.check_user_is_new(db, user_id.id):
raise UserNotFound()
Copy link
Collaborator

Choose a reason for hiding this comment

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

So… if the user is not new, you're simply refusing the operation, regardless of the condition?

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