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

[5.0.0 Breaking change] Using user defined value type for timers #2962

Closed
wants to merge 2 commits into from

Conversation

Amxx
Copy link
Collaborator

@Amxx Amxx commented Nov 11, 2021

The structures used in the timers library prevent storage packing (in particular, this affects Governor's Proposals).

This has to effect that makes it a breaking change:

  • Storage incompatibility (data is now packed) in the Governor contract.
  • New timers being "value type", makes it impossible to write "setDeadline" and "reset" methods.

PR Checklist

  • Tests
  • Documentation
  • Changelog entry

@Amxx Amxx added the breaking change Changes that break backwards compatibility of the public API. label Nov 11, 2021
@frangio frangio added the on hold Put on hold for some reason that must be specified in a comment. label Mar 8, 2022
@frangio frangio added this to the 5.0 milestone Sep 19, 2022
@frangio
Copy link
Contributor

frangio commented Sep 19, 2022

This needs to be rebased on next-5.0.

@frangio
Copy link
Contributor

frangio commented Jan 19, 2023

@Amxx Do we still want to do this or is your current belief that it's better to use uint48 types instead of introducing the timer abstraction?

@Amxx
Copy link
Collaborator Author

Amxx commented Jan 19, 2023

In 5.0 we definitelly want to move away from struct-based timers has this prevents proper packing.

This leaves us with some options:

  • using UDVT (as shown in the PR)
    • the underlying type should probably be changed from uint64 to uint48
    • we'll continue to have both timestamp & blocknumber
  • remove the Timers library altogether

A third option might be to add an abstract "withClock" contract, that implement EIP-5805's clock(), a UDVT, and the internal function to compare that UDVT to the clock
(I'm not sure about this third option, need more thoughts)

@frangio
Copy link
Contributor

frangio commented Feb 24, 2023

Closing due to deprecatioon (#4062).

@frangio frangio closed this Feb 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Changes that break backwards compatibility of the public API. on hold Put on hold for some reason that must be specified in a comment.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants