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

Fix "max per sponsee" conditions #55

Merged
merged 5 commits into from
Mar 13, 2024
Merged

Conversation

aguillon
Copy link
Collaborator

@aguillon aguillon commented Mar 12, 2024

  • Wif notebook to test
  • Breaks the front-end, but I won't address that
  • No migrations because they are fucked at the moment

@aguillon
Copy link
Collaborator Author

#45

Copy link
Collaborator

@quentin-burg quentin-burg left a comment

Choose a reason for hiding this comment

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

I don't have tested the notebook and the frontend yet, I'll do that now 👀

MAX_CALLS_PER_ENTRYPOINT = "MAX_CALLS_PER_ENTRYPOINT"
# Max number of calls per sponsee per contract
Copy link
Collaborator

Choose a reason for hiding this comment

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

So it's not the same business rule which is mentionned in the PR #45 , right ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes it is, but apparently I can't get the wording right. :)

The goal is to limit the subsidy for new users, for the smart contract targeted by the condition. So we want users to be able to submit N operations to this smart contract, but not more. But this should not impact other users, including newer accounts.

I suppose that it could be "max number of calls per sponsee per vault" instead?

@@ -163,7 +157,7 @@ class Condition(Base):
created_at = Column(
DateTime(timezone=True), default=datetime.datetime.utcnow(), nullable=False
)

is_active = Column(Boolean, nullable=False)
Copy link
Collaborator

Choose a reason for hiding this comment

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

So you can activate or not a condition ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The idea is that conditions may expire for whatever reason, and should not be taken into account later on.

src/crud.py Outdated Show resolved Hide resolved
@@ -329,17 +329,18 @@ def create_max_calls_per_sponsee_condition(
db_condition = models.Condition(
**{
"type": schemas.ConditionType.MAX_CALLS_PER_SPONSEE,
"sponsee_address": condition.sponsee_address,
"contract_id": condition.contract_id,
Copy link
Collaborator

Choose a reason for hiding this comment

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

why you can't write a migration script ?
If you have a database with the last version of model you can write a migration file and execute it I think.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's more of a "I won't bother for now" thing.

@quentin-burg
Copy link
Collaborator

Very cool with the notebook :)

@aguillon
Copy link
Collaborator Author

Allez pouf

@aguillon aguillon merged commit d17ab38 into staging Mar 13, 2024
1 check passed
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