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: calculation should be UTC now instead of executor's now #397

Conversation

Ansonhkg
Copy link
Collaborator

Description

This PR addresses an issue in the calculateUTCMidnightExpiration function, where calculations for expiration timestamps were inaccurately computed for users in timezones ahead of UTC. Specifically, when attempting to calculate the expiration timestamp with a parameter like daysUntilUTCMidnightExpiration: 1, the result would inaccurately represent less than 24 hours until expiration due to the discrepancy between local and UTC times.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

minting rli with 1 day expiration

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Copy link
Contributor

@MaximusHaximus MaximusHaximus left a comment

Choose a reason for hiding this comment

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

Looks good, functionally, but I wonder about us doing date math on behalf of the user as there are some non-intuitive cases depending on the timezone of the caller, and if they pass '0' it will create an expiration date at the end of the prior month I think. Is it an absolute requirement that the quotas must be set to expire at UTC midnight specifically, from the systems perspective? If I'm running a service outside of UTC, this seems very awkward for me. Just for my info -- is there a technical limitation/reason why we don't just let the consumer of this API pass us an explicit timestamp and leave the date math logic up to them?

@Ansonhkg
Copy link
Collaborator Author

Looks good, functionally, but I wonder about us doing date math on behalf of the user as there are some non-intuitive cases depending on the timezone of the caller, and if they pass '0' it will create an expiration date at the end of the prior month I think. Is it an absolute requirement that the quotas must be set to expire at UTC midnight specifically, from the systems perspective? If I'm running a service outside of UTC, this seems very awkward for me. Just for my info -- is there a technical limitation/reason why we don't just let the consumer of this API pass us an explicit timestamp and leave the date math logic up to them?

Yea, the crypto world (ethereum, and all chains) runs on UTC so it's an absolute requirement, that's why the parameter is specifically named to daysUntil**UTC**MidnightExpiration

@Ansonhkg Ansonhkg merged commit 883d7c4 into staging/3.2.6 Mar 12, 2024
2 of 4 checks passed
@Ansonhkg Ansonhkg deleted the feature/lit-2654-js-sdk-fix-utc-calculation-on-expiration branch March 12, 2024 17:36
@MaximusHaximus
Copy link
Contributor

Looks good, functionally, but I wonder about us doing date math on behalf of the user as there are some non-intuitive cases depending on the timezone of the caller, and if they pass '0' it will create an expiration date at the end of the prior month I think. Is it an absolute requirement that the quotas must be set to expire at UTC midnight specifically, from the systems perspective? If I'm running a service outside of UTC, this seems very awkward for me. Just for my info -- is there a technical limitation/reason why we don't just let the consumer of this API pass us an explicit timestamp and leave the date math logic up to them?

Yea, the crypto world (ethereum, and all chains) runs on UTC so it's an absolute requirement, that's why the parameter is specifically named to daysUntil**UTC**MidnightExpiration

I understand that the expiresAt timestamps must defined as UTC values -- what I'm asking is if there is a technical reason why we can't allow those to expire at a time other than at midnight in UTC.

For example, if I want to set a quota that expires at midnight in my or my end-user's local timezone, and they or I am in UTC -2, then I would want to set the expiration timestamp to UTC 0200 instead of 0000; otherwise it would expire at 10PM in my or my user's timezone which may surprise them or me.

To re-phrase, Did we arbitrarily choose that all expirations will happen at midnight in the UTC timezone, and only provide an API that forces the users to define their expirations that way, or is it a technical requirement that all expirations will happen at midnight in the UTC timezone?

@MaximusHaximus
Copy link
Contributor

MaximusHaximus commented Mar 13, 2024

Just for later reference: UTC expirations are enforced at the smart contract logic-level so this is a technical requirement currently (the smart contract will throw an error if you set an expiresAt to anything other than midnight on a particular date) -- ref: https://github.com/LIT-Protocol/lit-assets/blob/60c4144d24cf49e9cee6c65d08a052ea5df34290/blockchain/contracts/contracts/lit-node/RateLimitNFT/RateLimitNFTFacet.sol#L169-L172

Thanks for the info @Ansonhkg :)

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