feat(world-id): defensively claim unmigrated apps' on-chain rp_ids - #2238
feat(world-id): defensively claim unmigrated apps' on-chain rp_ids#2238Gr1dlock wants to merge 12 commits into
Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06615e9446
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * a row that polls forever. Do not claim ids for apps expected to self-manage | ||
| * until that flow lands. | ||
| */ | ||
| export async function POST(request: NextRequest) { |
There was a problem hiding this comment.
In the inspected app-router setup, handlers under web/api are only reachable when a matching web/app/api/.../route.ts re-exports them, and the existing leading-underscore endpoints use %5F... route folders. I searched web/app/api and there is no web/app/api/%5Fpre-register-rp-ids/route.ts, so POST /api/_pre-register-rp-ids will 404 in Next instead of invoking this new handler; the operator tool added here only works in direct unit tests until the route export is added.
Useful? React with 👍 / 👎.
| const client = await getAPIServiceGraphqlClient(); | ||
| const results: { app_id: string; outcome: Outcome; rp_id?: string }[] = []; | ||
|
|
||
| for (const appId of appIds) { |
There was a problem hiding this comment.
When an internal caller submits the same app_id more than once with dry_run: false, this loop processes the raw array repeatedly. Because submitRegisterRpTransaction submits a UserOp and returns before the first registration is mined, a later duplicate can still read the chain as uninitialized, submit another register for the same rp_id with a fresh nonce, and report another claimed, undermining the endpoint's gas-spend safeguards. Dedupe or reject duplicate app_ids before entering the loop.
Useful? React with 👍 / 👎.
06615e9 to
6509c20
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7e914861b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| parseRpId(rpIdString), | ||
| primaryConfig.contractAddress, | ||
| ); | ||
| if (onChain.initialized) { |
There was a problem hiding this comment.
When the developer chooses the self-managed flow and submits the displayed register() transaction from their own wallet before clicking Continue, this new guard sees the RP as initialized and returns rp_id_taken instead of creating the Portal row. I inspected the self-managed transaction step: it shows the contract/function details and the Continue action calls this register_rp path, so a user who completes the on-chain registration first can no longer finish setup or get a row for status/proof-context reconciliation. Consider aligning the ordering (create the row before instructing the transaction) or distinguishing the expected self-managed completion from a squatter/pre-claim.
Useful? React with 👍 / 👎.
|
@codex review |
d7e9148 to
9990bb7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9990bb7030
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ourManagerAddress && | ||
| addressesEqual(onChain.manager, ourManagerAddress) |
There was a problem hiding this comment.
When a self-managed app's rp_id is already initialized and resolveManagerAddress() returns null during a KMS outage or shared-key misconfiguration, this condition evaluates false and the handler continues to insert a self_managed row even if the manager is actually the Portal pre-claim. That defeats the intended rp_id_taken guard for pre-claimed ids; because rp-status trusts self-managed rows by mode, the row can later be promoted against the placeholder signer and leave the developer with an unusable registration instead of a retryable/support error. Treat an initialized RP with an unresolved Portal manager as unknown/retryable rather than falling through.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d006c08b7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // by this PR since it stores no expected roles for self-managed rows. | ||
| const primaryConfig = getRpRegistryConfig(); | ||
| const placeholderSigner = process.env.RP_ID_PRE_REGISTRATION_SIGNER; | ||
| if (primaryConfig && placeholderSigner) { |
There was a problem hiding this comment.
The updated placeholder-signer guard still fails open when RP_ID_PRE_REGISTRATION_SIGNER is unset/malformed, or when getRpRegistryConfig() returns null because of an unrelated RP env var: the whole on-chain check is skipped and the code proceeds to insert a self_managed row. For any app already defensively claimed by Portal, rp-status trusts self-managed rows by mode, so that row can be promoted against the placeholder signer and leave the developer with an unusable registration; this path should return a retryable/config error instead of bypassing the check once pre-claims can exist.
Useful? React with 👍 / 👎.
| adoptExistingClaim = Boolean( | ||
| ourManagerAddress && | ||
| addressesEqual(existingOnChainRp.manager, ourManagerAddress), | ||
| ); |
There was a problem hiding this comment.
When an app's rp_id is initialized because Portal pre-claimed it, a transient KMS failure in resolveManagerAddress() returns null, and this Boolean conversion turns that unknown state into adoptExistingClaim = false. The next branch reports rp_id_taken by a foreign manager before the retryable KMS path, so managed onboarding for pre-claimed apps is blocked with a permanent/support error during a KMS outage or shared-key misconfiguration; return a KMS/config error when the shared manager cannot be resolved instead of classifying it as foreign.
Useful? React with 👍 / 👎.
|
@codex review |
3d006c0 to
0432b1f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0432b1fa82
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| try { | ||
| const kmsClient = await getKMSClient(config.kmsRegion); | ||
| const operationHash = await submitRegisterRpTransaction(config, { |
There was a problem hiding this comment.
When NEXT_PUBLIC_APP_ENV=production and the RP_REGISTRY_STAGING_* vars are configured, the managed registration flow mirrors production apps onto the staging registry, but this defensive sweep submits a claim only against the primary getRpRegistryConfig() contract. The endpoint can therefore report an app as claimed while leaving the same predictable rp_id free on the staging mirror, so a squatter can still take that side and make the later migration's staging registration fail; the defensive claim/adoption path should cover the configured staging registry as well.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7977b036fc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * Defaults to true: the only way to spend gas is to ask for it explicitly. | ||
| * A dry run reports exactly what a real run would submit. | ||
| */ | ||
| dry_run: yup.boolean().default(true), |
There was a problem hiding this comment.
When an internal caller sends dry_run as the string "false" (for example from a hand-built JSON/CLI wrapper), Yup casts it to boolean false before validation, so this endpoint submits real claims even though the request did not explicitly pass the JSON boolean false. Since the new endpoint’s main safety property is that gas spending requires an explicit opt-out from dry-run, make this boolean strict or otherwise reject non-boolean values.
Useful? React with 👍 / 👎.
Salting new rp_ids cannot protect the installed base: every app created so far has a public app_id, so its uint64(keccak256(app_id)) rp_id is predictable forever, and on-chain register() is permissionless, zero-fee and first-come with no reclaim path (H1 #3910854). The only defense left for those apps is to hold the id ourselves until the app is ready to use it. POST /api/_pre-register-rp-ids claims the ids of a caller-supplied list of apps, registering each to the Portal's shared manager key with a placeholder signer. It creates no rp_registration rows — on-chain state is the record of what we hold, and inventing rows would make the Portal claim apps are registered when their owners never asked, which is what proof-context serves from. Because every claim spends L2 gas and the registry's WLD fee, the safeguards are the point: a kill switch that makes the endpoint inert, dry-run by default so spending requires asking for it, a hard 25-app ceiling per call, and a per-outcome breakdown in the response and the logs so a run that skipped everything for an unexpected reason cannot read as a successful sweep. Claims never happen on a failed on-chain read, and re-running is idempotent. Adoption is what keeps a claimed app onboardable. submitManagedRpRegistration already refused an rp_id that was taken on-chain; it now distinguishes a squatter from our own claim by the manager address — the one role only our KMS key can sign for — and rotates the placeholder signer to the developer's real one instead of submitting a register() the contract would reject. Adoption is pinned to the shared key regardless of ENABLE_SHARED_KEY_RP_REGISTRATION, because a dedicated key would be a manager the contract has never seen and every later update would revert. Self-managed apps cannot adopt yet: the developer registers from their own wallet, so a claimed id means their register() reverts. register_rp now fails that case loudly with rp_id_taken instead of inserting a row that polls pending forever. Transferring the manager to the developer is the missing piece (submitTransferManagerTransaction exists, nothing drives it), so ids should not be claimed for apps expected to self-manage until that lands. Co-Authored-By: Claude <noreply@anthropic.com>
Two Codex findings, both real: Handlers under web/api are only reachable through a matching app-router re-export, and the leading-underscore endpoints use %5F-escaped route folders (see web/app/api/%5Fdeactivate-deleted-app-rps). Without one, POST /api/_pre-register-rp-ids 404s and the tool only ever worked from unit tests. `next build` now lists the route next to the existing cron. A repeated app_id was processed twice. submitRegisterRpTransaction returns once the UserOp is submitted rather than mined, so the second pass would read the chain as still uninitialized, submit another register() for the same rp_id with a fresh nonce, and report a second claim — spending gas twice and defeating the per-call ceiling the endpoint exists to enforce. Deduped before the loop, and the drop is logged rather than silent. Co-Authored-By: Claude <noreply@anthropic.com>
The guard added with pre-registration failed any self-managed rp_id that was already initialized on-chain. That is backwards: the self-managed developer runs register() from their own wallet on the instructions screen, and this mutation is the "Continue" that follows — so an initialized id is the HEALTHY state, and the guard would have broken every legitimate self-managed completion, leaving those apps with no row for status or proof-context reconciliation. Only an id held by the Portal's own shared manager should fail: that is the defensive pre-claim, the developer's register() reverted against it, and handing it over needs a manager transfer no flow drives yet. Any other manager is the developer's own registration — or a squatter's, which for self-managed the Portal cannot distinguish either way, unchanged by this PR since it stores no expected roles for self-managed rows. Tests now pin both directions, since the existing suite passed only because the guard is skipped when no shared manager key is configured. Codex independently flagged the same inversion. Co-Authored-By: Claude <noreply@anthropic.com>
The self-managed guard identified a Portal pre-claim by resolving our shared manager key and comparing addresses. Codex pointed out the fail-open: when resolveManagerAddress returns null during a KMS outage the comparison is false, the handler inserts the row anyway, and because rp-status trusts self-managed rows by mode it later promotes that row against the pre-claim's placeholder signer — leaving the developer a registration that can never sign. Failing closed instead would be worse. An initialized rp_id is the NORMAL state at this point (the developer just registered it themselves), so requiring a resolved manager would break every legitimate self-managed completion whenever KMS is unavailable — and KMS has no business in this flow at all, since the Portal holds no keys for self-managed apps. Compare the placeholder signer instead. It is a plain env var we control, no remote call is involved, so the check cannot fail open or fail closed on someone else's outage. A pre-claim is the only thing that carries it: only we could rotate it away, and doing so creates a row, which this path answers with already_registered from the DB claim. Co-Authored-By: Claude <noreply@anthropic.com>
…rror Two Codex findings, both the same shape: an unknown state was being reported as something permanent. Managed adoption converted an unresolved shared manager address straight to "not ours", so a transient KMS failure told the developer of a pre-claimed app that someone else owns their rp_id and to contact support. Unresolvable is now its own outcome — a retryable kms_error, with the claimed slot released so the retry is not met with already_registered. The self-managed placeholder-signer guard failed open whenever it could not run: no placeholder configured, or getRpRegistryConfig() null for an unrelated reason. For an app the Portal has pre-claimed that inserts a row which rp-status promotes (it trusts self-managed rows by mode) against a signer that can never sign. What skipping means now depends on whether pre-claims can exist at all: - pre-registration enabled but misconfigured -> config_error, a deploy fault - placeholder set but the chain unreadable -> rpc_error, retryable - placeholder unset, never enabled -> skip; no claims exist That last case is every environment that has not run the tool, so it must stay open — the alternative is making self-managed registration depend on config it has no reason to need. Noted the operational invariant in the code: once pre-registration has run somewhere, RP_ID_PRE_REGISTRATION_SIGNER has to stay set there, because the claims outlive the flag. Rebased onto #2221, resolving the overlap where the collision check moved after the DB claim: adoption keeps the slot, both failure paths release it. Co-Authored-By: Claude <noreply@anthropic.com>
A managed registration mirrors onto the staging registry on production deployments, but the defensive sweep only claimed the primary contract. That left the same predictable rp_id free on the staging side for a squatter to take, which then makes the real migration's staging registration revert with IdAlreadyInUse and records staging `failed` — and the endpoint had already reported the app as `claimed`, so the gap was invisible. The sweep now walks both configured registries and reports per-registry outcomes, so a half-claimed app shows up as such in the counts rather than collapsing into a single `claimed`. The staging mirror in submitManagedRpRegistration gained the same adoption branch as production, decided per contract because the two registries are claimed independently and can legitimately disagree. Raised as a P2 by Codex. Co-Authored-By: Claude <noreply@anthropic.com>
yup cast the string "false" to boolean false, so a hand-built curl or wrapper script could make this endpoint spend real gas without ever passing a JSON boolean — and explicit opt-out is the whole safety property. Not fixed with .strict(): yup applies defaults during casting, which strict mode skips, so .strict().default(true) leaves an ABSENT dry_run as undefined and therefore falsy. That inverts the guarantee instead of tightening it — the local test suite caught it immediately. The check now runs on the raw body before yup sees it, and a test pins the omitted case so the same trap cannot be reintroduced. Also drops a duplicated foreign-manager block the previous rebase left behind. It sat after the adoption branch and returned rp_id_taken unconditionally, so adoption could never take effect — caught by the adoption test failing. Rebased onto #2221's slot-release fix; the release is now a shared helper used by both the kms_error and rp_id_taken paths. Co-Authored-By: Claude <noreply@anthropic.com>
|
@codex review |
7977b03 to
8576ea9
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8576ea905d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| adoptStagingClaim = | ||
| existingStagingRp.initialized && | ||
| addressesEqual(existingStagingRp.manager, managerAddress); |
There was a problem hiding this comment.
When the defensive sweep only succeeds on staging (for example, the production read/submission failed but the staging claim succeeded), a later production registration with shared-key mode off creates a dedicated managerAddress. This check compares the staging manager only to that dedicated manager, so a staging RP already held by Portal's shared pre-claim is treated as not adopted and the code falls through to register() on an initialized id; the row then stores the dedicated manager key, and staging retry/status paths see the shared-manager claim as foreign, leaving staging permanently failed. Detect the shared staging pre-claim or choose the shared manager for the whole registration before submitting.
Useful? React with 👍 / 👎.
…production The sweep can succeed on staging and fail on production. A later managed registration in dedicated-key mode then minted a dedicated manager for production and only afterwards compared staging against it — so a staging rp_id already held by our shared pre-claim looked foreign, register() reverted on an initialized id, and staging was left permanently failed. Every subsequent staging status check and retry compares against the dedicated key recorded on the row and reads the shared pre-claim as foreign too, so it never recovers. The row stores ONE manager_kms_key_id, so a pre-claim on either registry has to decide the key for both. Staging is now read before the key is chosen, and a staging-only pre-claim forces the shared key just as a production one does. Both adoption flags are then just consulted at submission time. Only production remains authoritative for whether the app can register at all — a foreign staging claim means staging cannot mirror, not that onboarding is blocked. Verified the new test fails without the fix and passes with it, rather than assuming it discriminates. Raised as a P2 by Codex. Co-Authored-By: Claude <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9630f600c4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| try { | ||
| const kmsClient = await getKMSClient(registry.config.kmsRegion); | ||
| const operationHash = await submitRegisterRpTransaction( |
There was a problem hiding this comment.
When an operator repeats a real run for the same app before the previous UserOp has mined or become visible to getRpFromContract, the next call still sees the id as uninitialized and submits another register() here; the current dedupe only handles duplicates within one JSON payload. Since the UserOp nonce includes per-attempt randomness, the duplicate can be accepted concurrently and one will later revert/burn gas, so the endpoint should persist/lock pending claims or otherwise skip recently submitted rp_ids until the first operation settles.
Useful? React with 👍 / 👎.
| } catch (error) { | ||
| // Non-fatal for the same reason as production: staging is a best-effort | ||
| // mirror, so a failed read falls through to registering. | ||
| logger.warn("Could not pre-check the staging rp_id; registering", { |
There was a problem hiding this comment.
When production has a staging registry configured and this staging pre-check throws while the staging rp_id is already held by Portal's shared pre-claim, the catch leaves existingStagingRp null and adoptStagingClaim false. With shared-key mode off the flow then stores a dedicated manager after production succeeds, the staging register() reverts, and the staging retry path rejects the real shared on-chain manager as foreign, leaving that side unrecoverable without manual repair; fail/retry the registration or choose the shared key when staging ownership is unknown and pre-claims are possible.
Useful? React with 👍 / 👎.
…nership Two Codex findings on the pre-registration path. An operator repeating a real run before the previous UserOp mined resubmitted: submitRegisterRpTransaction returns on submission, the on-chain read still shows the id as free, and the UserOp nonce carries per-attempt randomness — so both can be accepted and one later reverts, burning gas. The payload-level dedupe never covered this. A Redis marker now reserves each rp_id per registry for the UserOp validity window plus margin, mirroring the throttle in rp-status, and reports skipped_claim_in_flight. Fails OPEN without Redis: this is an operator tool that dry-runs by default, so refusing to work without a cache would be worse than the duplicate it prevents, and the on-chain read still catches anything mined. The staging pre-check's non-fatal catch left ownership as "not ours". With dedicated keys that mints a dedicated manager, staging's register() then reverts on an initialized id, and every later staging status check and retry compares against the dedicated key and reads the shared pre-claim as foreign — that side never recovers. Unknown now forces the shared key, which is always valid for a fresh registration. Scoped to environments that actually pre-claim (they keep RP_ID_PRE_REGISTRATION_SIGNER set), so dedicated keys keep their isolation everywhere else, and a staging RPC blip still never blocks a production registration. Both new tests were checked against the un-fixed code first to confirm they actually fail without the change. Co-Authored-By: Claude <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 45fa93ca91
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| : await submitRegisterRpTransaction(primaryConfig, { | ||
| rpId, | ||
| managerAddress, | ||
| signerAddress, | ||
| appName, |
There was a problem hiding this comment.
When an operator has just submitted a real pre-registration claim and a developer starts managed registration before that UserOp is mined, the on-chain read still looks uninitialized and this branch submits a competing register(). The in-flight Redis marker is only used in _pre-register-rp-ids (searching rp_claim_in_flight finds no reader here), so with shared-key mode off the DB row can record a dedicated manager while the pending claim wins on-chain with the shared manager; status/retry then classify the shared-manager claim as foreign instead of adopting it. Check the in-flight marker here or wait/force the shared adoption path while a claim is settling.
Useful? React with 👍 / 👎.
| } catch (error) { | ||
| logger.error("Failed to claim rp_id", { | ||
| error, | ||
| app_id: appId, | ||
| rpIdString, |
There was a problem hiding this comment.
When getKMSClient or submitRegisterRpTransaction throws before a UserOp is accepted, the Redis reservation created just before this block is left in place. A repeat real run for the same registry/RP during the full validity TTL then reports skipped_claim_in_flight even though there is no in-flight operation, leaving the id unclaimed by the tool until expiry; delete the reservation on this failure path or only keep it after a successful submission.
Useful? React with 👍 / 👎.
…it on failure The marker added last round had no reader outside the claim tool, so a developer starting managed registration while an operator's claim was still settling saw the id as free and submitted a competing register(). If the pre-claim won on-chain with the shared manager while the row recorded a dedicated one, every later status check and retry read the shared claim as foreign and the registration never reconciled. Registration now checks the marker and asks the developer to retry — by then the claim is visible and the adoption path takes over. Reuses the retryable submission_error code rather than adding a new one to four error maps. Writer and reader now live in one module (rp-claims.ts) so the key format and TTL cannot drift between them. A marker nobody sees is exactly the failure this round was about. Also releases the reservation when getKMSClient or the submission throws before a UserOp is accepted. Otherwise a repeat run reported skipped_claim_in_flight for the full TTL on an operation that never existed, leaving the id unclaimed. (P3.) Both new tests were checked against the un-fixed code first; the first one passed initially only because it reserved the wrong key, which is why that check matters. Co-Authored-By: Claude <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c255ccd4c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if ( | ||
| !existingOnChainRp?.initialized && | ||
| (await isClaimInFlight("production", rpIdString)) |
There was a problem hiding this comment.
When production has a staging registry and _pre-register-rp-ids has submitted only the staging-side claim (or the production side is already settled) but it has not mined yet, the staging pre-check still reads the id as free and this guard only waits on the production reservation. The flow can then mint/store a dedicated manager and submit a competing staging register(), so if the shared-manager pre-claim lands first, staging status/retry will classify it as foreign and leave staging unrecoverable. Fresh evidence in this version is that the claim writer reserves registry.label for both production and staging, while this reader checks only "production".
Useful? React with 👍 / 👎.
| const sharedManagerKeyId = process.env.RP_REGISTRY_MANAGER_KMS_KEY_ID; | ||
| const ourManagerAddress = sharedManagerKeyId | ||
| ? await resolveManagerAddress(sharedManagerKeyId, primaryConfig.kmsRegion) | ||
| : null; | ||
|
|
||
| if (sharedManagerKeyId && !ourManagerAddress) { |
There was a problem hiding this comment.
If an initialized RP was defensively pre-claimed but RP_REGISTRY_MANAGER_KMS_KEY_ID is absent during registration, this condition does not take the retryable kms_error path because sharedManagerKeyId is falsy; adoptExistingClaim stays false and the production ownership branch below reports a terminal rp_id_taken. A missing shared manager key is a deploy/config outage rather than proof of foreign ownership, so pre-claimed apps should get a retryable config/KMS error instead of being sent to support. Fresh evidence in this version is that the guard now handles unresolved addresses only when the env var is present.
Useful? React with 👍 / 👎.
…missing keys Two more gaps in the same decision, both the incompleteness this file keeps producing. The in-flight reader checked only "production" while the writer reserves both registries, so a settling staging claim let a dedicated key be minted and a competing staging register() go out; if the shared-manager pre-claim landed first, staging status and retry read it as foreign and that side never recovered. A settling staging claim now forces the shared key — staging is best-effort, so it does not fail the registration, matching how an unreadable staging registry is already handled. A missing RP_REGISTRY_MANAGER_KMS_KEY_ID also fell through to a terminal rp_id_taken, because the retryable guard only fired when the variable was present. For a pre-claimed app that reports a deploy problem as "someone else owns your id, contact support". Absent-but-pre-claims-possible is now a retryable config_error, while a foreign id in an environment that never pre-claimed still gets rp_id_taken — pinned by a test that passes either way on purpose, since it guards behaviour that must not change. Co-Authored-By: Claude <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 598e996df3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| !isZeroAddress(placeholderSigner), | ||
| ); | ||
|
|
||
| if (!canRecognisePreClaims && preRegistrationEnabled) { |
There was a problem hiding this comment.
When the pre-registration sweep has already run and the kill switch is turned back off, this guard is false even if RP_ID_PRE_REGISTRATION_SIGNER is missing/malformed or getRpRegistryConfig() is null. Fresh evidence in this version is that the self-managed fail-closed path is keyed only to ENABLE_RP_ID_PRE_REGISTRATION, but defensive claims outlive that flag; a Portal-held placeholder RP can then be inserted as self_managed, and rp-status trusts self-managed rows by mode, leaving the developer with an unusable registration instead of a retryable config error.
Useful? React with 👍 / 👎.
The self-managed guard's fail-closed path was keyed to ENABLE_RP_ID_PRE_REGISTRATION — the kill switch — while the comment three lines above it stated that defensive claims outlive that flag. So the exact state the comment described as an invariant (sweep has run, flag turned back off, placeholder unset) was silently unguarded: a Portal-held placeholder RP would be inserted as self_managed, and rp-status trusts self-managed rows by mode, so it would be promoted against a signer that can never sign. There are two independent tells that Portal holds an id, and the guard now uses both in cost order. The placeholder signer stays preferred: a plain env var, no remote call, so it cannot fail open or closed on someone else's outage. The shared manager address is the fallback, paid for only when the cheap tell is unavailable or negative — KMS has no business in a self-managed registration, but a silently broken registration is worse than a retryable error. Unresolvable manager is retryable, not terminal. With neither tell there is no in-band signal left, so the guard cannot run: a config error while claims are being made, a no-op where nothing was ever claimed. Verified the fallback test fails without the fallback, and that the existing "does not depend on KMS" test still passes with the fallback removed — so the cheap path is genuinely still preferred rather than incidentally exercised. Co-Authored-By: Claude <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b5957497d6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| managerKmsKeyId, | ||
| kmsClient, | ||
| }) | ||
| : await submitRegisterRpTransaction(primaryConfig, { |
There was a problem hiding this comment.
When the production ownership read above throws while pre-claims can exist and shared-key mode is off, existingOnChainRp stays null, so this fallback submits register() with a newly-created dedicated manager instead of treating ownership as unknown. If the app's id was already defensively claimed by Portal, that is the wrong operation for an initialized id; once recorded, status/retry compare the on-chain shared manager to the stored dedicated key and classify our own pre-claim as foreign, leaving onboarding stuck. Return a retryable RPC/unknown-ownership error before registering when pre-claim adoption cannot be determined.
Useful? React with 👍 / 👎.
| continue; | ||
| } | ||
|
|
||
| if (dryRun) { |
There was a problem hiding this comment.
When a real run just submitted a claim and the Redis reservation still exists, this dry-run branch runs before the reservation check below; because the on-chain read still shows the id as free, the response says would_claim even though the matching real run would return skipped_claim_in_flight. That undermines the dry-run-as-preview safeguard operators rely on before spending gas, so read the in-flight marker before emitting would_claim.
Useful? React with 👍 / 👎.
1 participant
PR Type
Description
Salting new rp_ids (#2237) cannot protect the installed base. Every app that already exists has a public
app_id, so itsuint64(keccak256(app_id))rp_id is predictable forever, and on-chainregister()is permissionless, zero-fee, first-come, with no reclaim path (H1 #3910854). For those apps the only defense left is to hold the id ourselves until the app is ready to use it — Takis's short-term suggestion in the thread.POST /api/_pre-register-rp-idsclaims the ids of a caller-supplied list of apps, registering each to the Portal's shared manager key with a placeholder signer.It creates no
rp_registrationrows. On-chain state is the record of what we hold. Inventing rows would make the Portal assert that apps are registered when their owners never asked, andproof-contextserves off exactly that row.Safeguards are the substance here, since every claim spends L2 gas and pulls the registry's WLD fee from our Safe:
ENABLE_RP_ID_PRE_REGISTRATIONkill switch — off makes the endpoint inert regardless of arguments.dry_run: false; a dry run reports exactly what a real run would submit.error— it's the single most important thing to surface, and the contract gives us no way to fix it.Adoption — what keeps a claimed app onboardable
Claiming an id would otherwise permanently lock the app out of registration.
submitManagedRpRegistrationalready refused an rp_id taken on-chain (#2221); it now tells a squatter from our own claim by the manager address — the one role only our KMS key can sign for — and rotates the placeholder signer to the developer's real one instead of submitting aregister()the contract would reject withIdAlreadyInUse. Adoption is pinned to the shared key regardless ofENABLE_SHARED_KEY_RP_REGISTRATION: a dedicated key would be a manager the contract has never seen, and every laterupdateRpwould revert.Known gap, and why the endpoint takes an explicit app list
Self-managed apps cannot adopt yet. A self-managed developer registers from their own wallet, so a claimed id means their
register()reverts.register_rpnow fails that case loudly withrp_id_takeninstead of inserting a row that pollspendingforever — but the real fix is transferring the manager to the developer, and whilesubmitTransferManagerTransactionexists, no flow drives it.That's why this is an operator-driven, explicit-app-list tool rather than a blanket sweep over every unmigrated app. A blanket sweep would silently break self-managed onboarding for everyone it touched, and it's on-chain and irreversible. Do not claim ids for apps expected to self-manage until the transfer flow lands — that sequencing is a product call, not something this PR should decide.
Notes for review
GetAppInfo/FetchRpRegistrationqueries so no.graphqlcodegen (which needs a live Hasura) is required.addressesEqualis now exported fromrp-utilsso the ownership check can't drift into its own.toLowerCase()comparison that skips normalization.Eleven rounds of automated review produced 14 findings, seven of them consecutively
in one place: the rp_id ownership decision in
submitManagedRpRegistration. Thatdecision is now ~105 lines over seven interacting inputs (production initialized /
staging initialized / staging readable / shared key configured / manager resolvable /
production claim in flight / staging claim in flight), and each round has found a
different unhandled cell of that matrix. Two are still open:
throws while pre-claims can exist and dedicated-key mode is on, the flow registers
with a fresh dedicated manager. If the id was in fact pre-claimed, status and retry
then compare the on-chain shared manager against the stored dedicated key and
classify our own claim as foreign, leaving onboarding stuck until manual repair.
would_claimwhere the matching real run would reportskipped_claim_in_flight.That weakens dry-run-as-preview, which is the safeguard operators rely on.
I stopped patching individual cells deliberately. The change that terminates this is
extracting the ownership decision into a pure function over its inputs and
table-testing the combinations, so correctness comes from a covered matrix rather
than from the next review round. That is a focused piece of work and it should
happen before this merges.
It is also worth deciding whether this PR is wanted at all first. #2237 already
makes every rp_id issued from now on unguessable; this PR only protects the existing
installed base, and only for apps an operator explicitly chooses to claim. If the
answer is "not broadly," most of the complexity above earns nothing. Self-managed
apps additionally cannot adopt a claimed id yet (see above), so claiming ids for
apps expected to self-manage is not safe until the manager-transfer flow lands.
Checklist