Skip to content

Conversation

@Dodecahedr0x
Copy link
Contributor

@Dodecahedr0x Dodecahedr0x commented Dec 2, 2025

/permission/force-update route has been removed after the refactoring of permission on the TEE. Now, instead of having the TEE subscribing to permissions on mainnet, it directly queries the colocated validator to get permissions using cloned data. This is a slightly slower approach (more network queries, especially for never-seen-before accounts) but much more robust with no chance of missing the permission.

Summary by CodeRabbit

  • Refactor
    • Simplified permission verification logic by eliminating unused retry mechanisms while maintaining existing API compatibility and error handling behavior. Permission checks now complete through a single polling cycle without fallback paths.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 2, 2025

Walkthrough

Removed the internal forcePermissionUpdate function and its fallback retry logic from waitUntilPermissionActive in two permission.ts files across different modules. The polling mechanism now relies solely on initial timeout-based polling without attempting forced updates.

Changes

Cohort / File(s) Summary
Permission polling simplification
ts/kit/src/access-control/permission.ts, ts/web3js/src/access-control/permission.ts
Removed forcePermissionUpdate function and eliminated the fallback retry path in waitUntilPermissionActive. Control flow now ends with a single timeout-based polling loop returning false, removing the force-update retry logic. Public API signatures unchanged.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Requires verification that removing the fallback mechanism doesn't introduce permission-checking gaps or race conditions
  • Need to trace the implications of eliminating forced updates on the permission authorization flow
  • The related PR history suggests this area has undergone multiple control flow iterations, warranting careful review of state transitions

Possibly related PRs

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main change: removal of the forcePermissionUpdate function and its fallback logic from the permission handling system across two files.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch dode/remove-force-update

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8883a4e and 9d806c1.

📒 Files selected for processing (2)
  • ts/kit/src/access-control/permission.ts (3 hunks)
  • ts/web3js/src/access-control/permission.ts (3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-26T11:05:25.930Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/ephemeral-rollups-sdk PR: 83
File: ts/kit/src/privacy/permission.ts:59-61
Timestamp: 2025-11-26T11:05:25.930Z
Learning: In ts/kit/src/privacy/permission.ts and ts/web3js/src/privacy/permission.ts, the `waitUntilPermissionGranted` function checks `if (!!authorizedUsers)` to determine if permission is granted. An empty array `[]` for `authorizedUsers` means the permission is set/granted, while `undefined` means the permission has not yet been observed by the TEE. The truthiness check is correct and should not be changed to check array length.

Applied to files:

  • ts/web3js/src/access-control/permission.ts
  • ts/kit/src/access-control/permission.ts
🧬 Code graph analysis (1)
ts/web3js/src/access-control/permission.ts (1)
ts/kit/src/access-control/permission.ts (1)
  • PermissionStatusResponse (3-5)
🔇 Additional comments (3)
ts/web3js/src/access-control/permission.ts (2)

51-76: LGTM - Force update removal aligns with new architecture.

The simplified polling logic correctly implements the new architecture described in the PR objectives, where the TEE directly queries the validator instead of using a subscription model that required force updates.


63-63: Verify semantic correctness of the length check.

Based on retrieved learnings, in similar permission functions an empty array [] for authorizedUsers means "permission granted" while undefined means "not yet observed." However, this function checks authorizedUsers.length > 0, which would return false for an empty array.

If waitUntilPermissionActive intentionally requires at least one authorized user (different semantics from "granted"), this is correct. Otherwise, consider using just the truthiness check: if (!!authorizedUsers).

Based on learnings, please confirm the intended behavior when authorizedUsers is an empty array.

ts/kit/src/access-control/permission.ts (1)

51-76: LGTM - Changes align with ts/web3js version.

This file mirrors the changes in ts/web3js/src/access-control/permission.ts, correctly removing the force update mechanism. The same approval and concerns from that file apply here:

  1. ✅ Force update removal aligns with new TEE architecture
  2. ⚠️ Verify that the authorizedUsers.length > 0 check (line 63) has the correct semantics per the retrieved learning about empty arrays
  3. 💡 Consider making the polling interval (line 71) configurable as suggested for the web3js version

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@jonasXchen jonasXchen merged commit 16ffc3c into main Dec 3, 2025
5 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.

3 participants