Uh oh!
There was an error while loading. Please reload this page.
Support Lit-claimed ORCID sandbox accounts - #100
Conversation
Lit oracle uses sourceId=4 with plain ORCID iDs for sandbox accounts, while legacy accounts use sourceId=2 with "sandbox-" prefixed names. This produces different on-chain account IDs. - Update orcidAccountIdUtils to recognize sourceId=4 as ORCID - Add getAllPossibleOrcidAccountIds to compute both legacy and Lit account IDs for backwards-compatible resolution - Update resolver to try all candidate account IDs
🚅 Deployed to the graphql-api-pr-100 environment in Drips App
|
There was a problem hiding this comment.
Pull request overview
This PR adds support for Lit-claimed ORCID sandbox accounts that use a different source ID (sourceId=4) than legacy ORCID accounts (sourceId=2). The Lit oracle creates sandbox accounts with plain ORCID IDs like "0009-...", while legacy sandbox accounts use the "sandbox-0009-..." prefix. The changes enable the resolver to compute and try all possible account IDs for backwards compatibility.
Changes:
- Introduces
ORCID_SOURCE_IDSarray [2, 4] replacing the singleORCID_FORGE_IDconstant - Adds
getAllPossibleOrcidAccountIdsfunction to generate both legacy and Lit account ID candidates - Updates
orcidLinkedIdentityByOrcidresolver to iterate through candidate account IDs and return the first match
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| src/orcid-account/orcidAccountIdUtils.ts | Replaces ORCID_FORGE_ID with ORCID_SOURCE_IDS array, updates extractSourceIdFromAccountId to properly decode 7-bit sourceId, and modifies isOrcidAccount to check multiple valid source IDs |
| src/common/dripsContracts.ts | Adds getAllPossibleOrcidAccountIds function to compute both legacy (sourceId=2) and Lit (sourceId=4) account IDs for a given ORCID |
| src/linked-identity/linkedIdentityResolvers.ts | Updates resolver to try multiple candidate account IDs sequentially, returning first match or falling back to unclaimed entry |
| tests/linked-identity/linkedIdentityResolvers.test.ts | Updates test mocks to use getAllPossibleOrcidAccountIds instead of getCrossChainOrcidAccountIdByOrcidId |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // No identity found — return a fake unclaimed entry using the first candidate | ||
| assertIsLinkedIdentityId(candidateAccountIds[0]); | ||
| return toFakeUnclaimedOrcid(orcid, candidateAccountIds[0], chain); |
There was a problem hiding this comment.
While getAllPossibleOrcidAccountIds always returns at least one account ID (the legacy sourceId=2 variant), the code would be more robust with an explicit check before accessing candidateAccountIds[0] on line 124. Consider adding an assertion or throw statement if the array is unexpectedly empty, or adding a comment explaining why the array is guaranteed to be non-empty. This improves code maintainability and makes the assumption explicit.
| for (const accountId of candidateAccountIds) { | ||
| assertIsLinkedIdentityId(accountId); | ||
| const identity = await linkedIdentitiesDataSource.getLinkedIdentityById( | ||
| accountId, | ||
| [chainToDbSchema[chain]], | ||
| ); | ||
| assertIsLinkedIdentityId(orcidAccountId); | ||
| const identity = await linkedIdentitiesDataSource.getLinkedIdentityById( | ||
| orcidAccountId, | ||
| [chainToDbSchema[chain]], | ||
| ); | ||
| if (identity) { | ||
| return toGqlLinkedIdentity(identity); | ||
| } | ||
| } |
There was a problem hiding this comment.
The resolver now makes sequential database queries for each candidate account ID until a match is found. In the worst case with sandbox ORCIDs, this means two sequential database lookups per request. For better performance, consider fetching all candidate account IDs in a single batched query using SQL IN clause or Promise.all with individual queries, then returning the first non-null result. This would reduce latency, especially on high-latency database connections.
| * - 2: regular ORCID | ||
| * - 4: sandbox ORCID (Lit oracle uses a separate sourceId instead of a name prefix) |
There was a problem hiding this comment.
The comment describing sourceId=2 as "regular ORCID" is misleading. According to the PR description and the code in getAllPossibleOrcidAccountIds, sourceId=2 is used for both regular production ORCID accounts AND legacy sandbox accounts (with "sandbox-" prefix). The comment should clarify that sourceId=2 is used for all legacy ORCID accounts (both production and sandbox with prefix), while sourceId=4 is specifically for Lit-claimed sandbox accounts without the prefix.
| *-2: regularORCID | |
| *-4: sandboxORCID(LitoracleusesaseparatesourceIdinsteadofanameprefix) | |
| *-2: legacyORCIDaccounts(bothproductionORCIDandlegacysandboxaccountswith"sandbox-"prefix) | |
| *-4: Lit-claimedsandboxORCIDaccountswithoutthe "sandbox-" prefix (use separate sourceId instead) |
| * - 2: regular ORCID | ||
| * - 4: sandbox ORCID (Lit oracle uses a separate sourceId instead of a name prefix) | ||
| */ | ||
| export const ORCID_SOURCE_IDS = [2, 4]; |
There was a problem hiding this comment.
The test file tests/orcid-account/orcidAccountIdUtils.test.ts imports ORCID_FORGE_ID which was removed in this PR and replaced with ORCID_SOURCE_IDS. This change will break the existing test suite when this PR is merged. The test file needs to be updated to import and test ORCID_SOURCE_IDS instead, and the test should verify that it contains both 2 and 4.
| const plainOrcid = orcidId.startsWith('sandbox-') | ||
| ? orcidId.slice('sandbox-'.length) | ||
| : orcidId; | ||
| const litSandboxAccountId = ( | ||
| await repoDriver.calcAccountId(4, ethers.toUtf8Bytes(plainOrcid)) | ||
| ).toString() as RepoDriverId; | ||
| if (litSandboxAccountId !== legacyAccountId) { | ||
| accountIds.push(litSandboxAccountId); |
There was a problem hiding this comment.
The function getAllPossibleOrcidAccountIds always adds sourceId=4 (Lit sandbox) candidates for production ORCID IDs that don't start with "sandbox-". However, based on the PR description, sourceId=4 is specifically for Lit-claimed sandbox accounts. This means for production ORCID IDs like "0000-0002-1825-0097", the function will compute and return a sourceId=4 account ID that would never be valid. Consider only computing the sourceId=4 account ID when the orcidId starts with "sandbox-" or when it looks like a sandbox ORCID pattern (starts with "0009-").
| constplainOrcid=orcidId.startsWith('sandbox-') | |
| ? orcidId.slice('sandbox-'.length) | |
| : orcidId; | |
| constlitSandboxAccountId=( | |
| awaitrepoDriver.calcAccountId(4,ethers.toUtf8Bytes(plainOrcid)) | |
| ).toString()asRepoDriverId; | |
| if(litSandboxAccountId!==legacyAccountId){ | |
| accountIds.push(litSandboxAccountId); | |
| // Only compute this for sandbox-style ORCID iDs: | |
| // - Legacy sandbox IDs: start with "sandbox-" | |
| // - Sandbox ORCID pattern: starts with "0009-" | |
| if(orcidId.startsWith('sandbox-')||orcidId.startsWith('0009-')){ | |
| constplainOrcid=orcidId.startsWith('sandbox-') | |
| ? orcidId.slice('sandbox-'.length) | |
| : orcidId; | |
| constlitSandboxAccountId=( | |
| awaitrepoDriver.calcAccountId(4,ethers.toUtf8Bytes(plainOrcid)) | |
| ).toString()asRepoDriverId; | |
| if(litSandboxAccountId!==legacyAccountId){ | |
| accountIds.push(litSandboxAccountId); | |
| } |
| export async function getAllPossibleOrcidAccountIds( | ||
| orcidId: string, | ||
| chainsToQuery: DbSchema[], | ||
| ): Promise<RepoDriverId[]> { | ||
| const availableChain = chainsToQuery.find( | ||
| (chain) => | ||
| dripsContracts[dbSchemaToChain[chain]] && | ||
| dripsContracts[dbSchemaToChain[chain]]!.repoDriver, | ||
| ); | ||
| if (!availableChain) { | ||
| throw new Error('No available chain with initialized contracts.'); | ||
| } | ||
| const { repoDriver } = dripsContracts[dbSchemaToChain[availableChain]]!; | ||
| const accountIds: RepoDriverId[] = []; | ||
| // Legacy account ID: sourceId=2, with the orcid string as provided | ||
| // (may include "sandbox-" prefix for legacy sandbox accounts) | ||
| const legacyAccountId = ( | ||
| await repoDriver.calcAccountId(2, ethers.toUtf8Bytes(orcidId)) | ||
| ).toString() as RepoDriverId; | ||
| accountIds.push(legacyAccountId); | ||
| // Lit sandbox account ID: sourceId=4, with plain ORCID (no "sandbox-" prefix) | ||
| const plainOrcid = orcidId.startsWith('sandbox-') | ||
| ? orcidId.slice('sandbox-'.length) | ||
| : orcidId; | ||
| const litSandboxAccountId = ( | ||
| await repoDriver.calcAccountId(4, ethers.toUtf8Bytes(plainOrcid)) | ||
| ).toString() as RepoDriverId; | ||
| if (litSandboxAccountId !== legacyAccountId) { | ||
| accountIds.push(litSandboxAccountId); | ||
| } | ||
| return accountIds; | ||
| } |
There was a problem hiding this comment.
The new function getAllPossibleOrcidAccountIds lacks direct unit test coverage. Given that this codebase uses comprehensive automated testing with vitest, and this is a critical function that handles ORCID account ID generation with complex logic for backwards compatibility, it should have dedicated unit tests. Consider adding tests that verify: 1) production ORCID IDs generate correct account IDs, 2) sandbox ORCID IDs with "sandbox-" prefix are handled correctly, 3) the deduplication logic works when both sourceIds produce the same account ID, and 4) error handling for unavailable chains.
| const candidateAccountIds = await getAllPossibleOrcidAccountIds(orcid, [ | ||
| chainToDbSchema[chain], | ||
| ]); | ||
| // Try each candidate account ID, return the first match found | ||
| for (const accountId of candidateAccountIds) { | ||
| assertIsLinkedIdentityId(accountId); | ||
| const identity = await linkedIdentitiesDataSource.getLinkedIdentityById( | ||
| accountId, | ||
| [chainToDbSchema[chain]], | ||
| ); | ||
| assertIsLinkedIdentityId(orcidAccountId); | ||
| const identity = await linkedIdentitiesDataSource.getLinkedIdentityById( | ||
| orcidAccountId, | ||
| [chainToDbSchema[chain]], | ||
| ); | ||
| if (identity) { | ||
| return toGqlLinkedIdentity(identity); | ||
| } | ||
| } | ||
| return identity | ||
| ? toGqlLinkedIdentity(identity) | ||
| : toFakeUnclaimedOrcid(orcid, orcidAccountId, chain); | ||
| // No identity found — return a fake unclaimed entry using the first candidate | ||
| assertIsLinkedIdentityId(candidateAccountIds[0]); | ||
| return toFakeUnclaimedOrcid(orcid, candidateAccountIds[0], chain); |
There was a problem hiding this comment.
The existing integration tests only mock getAllPossibleOrcidAccountIds to return a single account ID. To properly test the new multi-candidate resolution logic, additional test cases should be added that mock getAllPossibleOrcidAccountIds to return multiple candidate IDs and verify: 1) the resolver tries each candidate in order, 2) it returns the first match found, 3) when no match is found, it correctly falls back to toFakeUnclaimedOrcid with the first candidate. These scenarios are critical for ensuring the backwards compatibility feature works correctly.
Summary
getAllPossibleOrcidAccountIdsto compute both legacy and Lit account IDs for a given ORCID, enabling backwards-compatible resolutionorcidLinkedIdentityByOrcidresolver to try all candidate account IDs, returning the first matchContext: The Lit oracle creates sandbox ORCID accounts with
calcAccountId(4, "0009-...")(sourceId=4, plain ORCID iD), while legacy accounts usecalcAccountId(2, "sandbox-0009-...")(sourceId=2, prefixed name). These produce different on-chain account IDs. The resolver now tries both so either type of account can be resolved.Test plan
orcidLinkedIdentityByOrcid