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: create a increase_day_index function #23

Merged
merged 5 commits into from
Apr 10, 2024

Conversation

TAdev0
Copy link
Contributor

@TAdev0 TAdev0 commented Apr 10, 2024

This PR implements an increase_day_index function to increment a day_index variable in storage each time 24h have passed.

Closes #15

@TAdev0 TAdev0 requested a review from b-j-roberts as a code owner April 10, 2024 17:22
@TAdev0
Copy link
Contributor Author

TAdev0 commented Apr 10, 2024

@b-j-roberts Hi, here is a first draft.

I need more precisions about the purpose of this function. For now, i created an internal impl for this function, as i guess this function will be called internally each time a user interacts? Or is there any backend in charge of calling it externally by an admin ? or anyone?

Currently, the function will reset a new start day , setting the current block timestamp as a new start day time. This means that if the function is called more than each 24h exactly, each incrementation will not correspond to 24h exactly. I can modify it to make sure each index is 24h by setting the new start value to the last one + 24h

I will add tests once i fully understand how this function will be used!

@b-j-roberts
Copy link
Contributor

Hey, good work and nice points @TAdev0 !

I think having the function callable externally by anyone is good and decreases the need for trusting an admin. We will need to add an assert that prevents increasing the day index until enough time has passed though.
However, we will do the increase_day_index calls ourselves when the app is live, to make a smooth user experience.

For the "exact timing" issue, I think it's fine if each day goes longer than 24h exactly. However, it's up to you. Either have it set the new start time like it is now, or set the new start time to exactly start time + 24h, whatever you think is best!

Side note: You can set the initial start time in the constructor to be equal to creation time

@TAdev0
Copy link
Contributor Author

TAdev0 commented Apr 10, 2024

@b-j-roberts thanks for the quick answer.
True i forgot to initialise the initial the start_day_time value, my bad.

well it's up to you:

  • If you want a fix 24h period, i can change it. This will also mean that if nobody calls the function during multiple days, it will be possible to call it multiple time in a row to be up to date and catch the current real index day.

  • if we keep this, index will increase only by one once you call again the function

@b-j-roberts
Copy link
Contributor

@b-j-roberts thanks for the quick answer. True i forgot to initialise the initial the start_day_time value, my bad.

well it's up to you:

  • If you want a fix 24h period, i can change it. This will also mean that if nobody calls the function during multiple days, it will be possible to call it multiple time in a row to be up to date and catch the current real index day.
  • if we keep this, index will increase only by one once you call again the function

Yeah, based on the way Starknet works, I don't think there is a perfect solution for time based things like this sadly.
The trade-off with switching to an exact 24-hour period that allows multiple calls to catch up is that if that is done, people wouldn't have the opportunity to complete the quests on the skipped days.
So there will be something lost either way. Let's keep it how you have it now, unless you would really prefer the other option.
You can add the tests for the current implementation.

@TAdev0
Copy link
Contributor Author

TAdev0 commented Apr 10, 2024

oh yeah i see, indeed if you want each day index to be a period where people have the opportunity to complete quests we should keep it like this.

@TAdev0
Copy link
Contributor Author

TAdev0 commented Apr 10, 2024

@b-j-roberts just added 2 tests !

@b-j-roberts b-j-roberts merged commit 7379f2c into keep-starknet-strange:main Apr 10, 2024
3 checks 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.

[feat] Start a new day
2 participants