Skip to content

Feat/token upgrade timelock#572

Open
MerlinTheWhiz wants to merge 9 commits into
Iris-IV:mainfrom
MerlinTheWhiz:feat/token-upgrade-timelock
Open

Feat/token upgrade timelock#572
MerlinTheWhiz wants to merge 9 commits into
Iris-IV:mainfrom
MerlinTheWhiz:feat/token-upgrade-timelock

Conversation

@MerlinTheWhiz

Copy link
Copy Markdown

📌 Description

The 7-day timelock between propose_token_update and accept_token_update was already enforced in the contract logic. This PR hoists the magic number 7 * 86400 (and the underlying 86400) into named constants to address the finding in ROADMAP.md.

  • SECONDS_PER_DAY — standard day-to-seconds conversion factor
  • TOKEN_UPDATE_DELAY_SECS — the specific 7-day timelock duration for token updates

All existing usages of 86400 across production and test code are replaced with the appropriate constant.

🔗 Related Issues

Closes #562

🧪 Changes Made

  • Bug fix
  • New feature
  • Refactor
  • Documentation update

✅ Checklist

  • Code compiles successfully
  • Tests added/updated and passing
  • Linting passes (no warnings/errors)
  • Documentation updated (if required)
  • No breaking changes (or clearly documented)

⚠️ Breaking Changes

None. The constants resolve to the same values as the literal expressions they replace. All 274 existing tests pass.

📸 Screenshots (if applicable)

N/A

🧩 Additional Notes

The 7-day timelock for accept_token_update was already implemented as part of earlier work. The ROADMAP.md (§ Magic numbers scattered) specifically called out 7 * 86400 as a value that should be hoisted into a named constant. This PR resolves that technical debt while also extracting the general SECONDS_PER_DAY constant used in deadline, vesting, and other calculations.

@drips-wave

drips-wave Bot commented Jun 30, 2026

Copy link
Copy Markdown

@MerlinTheWhiz Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

@davidmaronio

Copy link
Copy Markdown
Contributor

the timelock itself is correct, propose_token_update sets release_after = now + TOKEN_UPDATE_DELAY_SECS (7d, checked_add, admin-auth'd), and accept_token_update enforces now >= release_after plus the escrow guard (blocks while active_campaign_count > 0 or total_raised_global != 0), with ttl bumps on both, no auth gap or bypass and no DataKey change so no storage orphaning. the actual diff here is mostly a value-preserving refactor swapping the magic 7*86400 for the named constant. fmt/clippy/test are green.

the main thing: this pr is a strict superset of both #569 and #571 (#569 in #571 in #572), so it also carries #571's dead-code PLATFORM_FEE_ABSOLUTE_MAX_BPS checks (unreachable, see my review on #571). since #572 already contains #569's good extension cap + the timelock refactor, the cleanest path is to land this one (rebased) and close #569/#571 as subsumed, but first either drop or fix those no-op fee checks so we're not merging dead code. rebase onto main and sort the fee hunk, then it's good to go.

@MerlinTheWhiz

Copy link
Copy Markdown
Author

@davidmaronio Yh the logic was mostly implemented, I just refactored with a couple changes like eliminating magic numbers and using variables for better scalability.

Also I worked on all three PRs, so it's like a chain branch. Each was checked out of the other with this being the parent/origin. So the right thing to do is to merge in order of PR raised starting from #569

I just rebased and I'm getting failed checks, and it's because you merged a PR with failed check to the project.

You can merge now.

@MerlinTheWhiz

Copy link
Copy Markdown
Author

Hi @davidmaronio this is still up

@davidmaronio

Copy link
Copy Markdown
Contributor

the timelock itself is still good. two things:

  1. the fee change here is the same one as Fix/platform fee upper bound #571 (replacing the 1000-bps PLATFORM_FEE_MAX_BPS cap with the 10000-bps one), which raises the effective ceiling from 10% to 100% and drops the lower bound, same question as on Fix/platform fee upper bound #571, please confirm that's intended or keep the 10% cap and add 10000 as an additional guard rather than a replacement.
  2. ci: fmt fails (Diff in admin.rs:4, create.rs:6, withdraw.rs:7, lib.rs:562, storage.rs:940), run cargo fmt. and clippy/test fail on the same merge-artifact breakage as fix(contract): enforce maximum campaign deadline extension #569/Fix/platform fee upper bound #571 (admin.rs:91 duplicate set_emergency_pause_signers + CreateCampaignParams missing fields). rebase instead of merge to clear it: git fetch origin && git rebase origin/main && git push --force-with-lease.
    since this pr is a superset of fix(contract): enforce maximum campaign deadline extension #569 and Fix/platform fee upper bound #571, sorting those out first will shrink this diff.

@davidmaronio

Copy link
Copy Markdown
Contributor

update: main was actually broken this whole time, not your pr. PR #574 (per-campaign tokens / milestone withdrawals / emergency pause) got merged on 07-01 while its CI was red and it didn't compile, so every branch that rebased or merged main inherited those exact errors (the duplicate set_emergency_pause_signers, the CreateCampaignParams missing-fields cascade, iter_mut, etc). that wasn't your merge, it was the base. i've reverted #574 in #584 and main is green again. so please rebase onto the current main and those inherited errors will clear: git fetch origin && git rebase origin/main && git push --force-with-lease. (#574's feature will be re-submitted properly.) sorry for the earlier note pinning it on your rebase.

@MerlinTheWhiz

Copy link
Copy Markdown
Author

@davidmaronio Alright then, please be on standby so you can review and merge once I'm done. You've not really being responsive, so I don't know when next you'll be active.

@MerlinTheWhiz

Copy link
Copy Markdown
Author

@davidmaronio I've implemented it. All done, and all green. Please review and merge.

@MerlinTheWhiz

Copy link
Copy Markdown
Author

@davidmaronio This is still up

@MerlinTheWhiz

Copy link
Copy Markdown
Author

@davidmaronio Hey, this is still up.

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.

[Feature] Implement timelocked token upgrades

2 participants