fix: normalize network names in provider functions#54
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPolygon ("pol") network support is introduced by normalizing the network name to "matic" before provider creation, then adding comprehensive token registry verification tests with Polygon-specific fixtures and assertions covering valid issuance, invalid registries, and issuer validation scenarios. ChangesPolygon token registry verification support
🎯 2 (Simple) | ⏱️ ~12 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/common/utils.ts (1)
33-34: ⚡ Quick winAdd test coverage for network name normalization.
The
normalizeNetworkNamefunction lacks test coverage. Consider adding tests to verify:
- Normalization of "pol" to "matic"
- Pass-through of other network names unchanged
- Case handling (once case-insensitivity is implemented)
- Integration with both
getDefaultProviderandgenerateProviderWould you like me to generate unit tests for the normalization function?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/common/utils.ts` around lines 33 - 34, Add unit tests for normalizeNetworkName: create tests that assert normalizeNetworkName("pol") === "matic" and that other inputs (e.g., "mainnet", "rinkeby") return unchanged values; add a case-insensitivity test once the function is updated (e.g., "Pol" -> "matic") or mark as pending if not implemented. Also add integration tests that call getDefaultProvider and generateProvider with network="pol" to ensure they internally use normalizeNetworkName and produce the same provider as when called with "matic". Reference the normalizeNetworkName function plus getDefaultProvider and generateProvider in your test file and use the existing test harness/assertion utilities used elsewhere in the repo.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/common/utils.ts`:
- Around line 33-34: The normalizeNetworkName function is currently
case-sensitive and won't map "POL"/"Pol"/other casings to "matic"; update
normalizeNetworkName to perform a case-insensitive comparison (e.g., normalize
input with network.toLowerCase()) and return "matic" when the lowercased value
equals "pol", otherwise return the original network; locate the helper named
normalizeNetworkName in src/common/utils.ts and adjust its logic accordingly.
---
Nitpick comments:
In `@src/common/utils.ts`:
- Around line 33-34: Add unit tests for normalizeNetworkName: create tests that
assert normalizeNetworkName("pol") === "matic" and that other inputs (e.g.,
"mainnet", "rinkeby") return unchanged values; add a case-insensitivity test
once the function is updated (e.g., "Pol" -> "matic") or mark as pending if not
implemented. Also add integration tests that call getDefaultProvider and
generateProvider with network="pol" to ensure they internally use
normalizeNetworkName and produce the same provider as when called with "matic".
Reference the normalizeNetworkName function plus getDefaultProvider and
generateProvider in your test file and use the existing test harness/assertion
utilities used elsewhere in the repo.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/verifiers/documentStatus/tokenRegistry/ethereumPolTokenRegistryStatus.test.ts`:
- Around line 7-8: The file ethereumPolTokenRegistryStatus.test.ts has Prettier
formatting violations (notably around the POL_RPC_URL declaration and several
test blocks) causing lint failures; run Prettier or the project's formatting
script on
src/verifiers/documentStatus/tokenRegistry/ethereumPolTokenRegistryStatus.test.ts
and fix the whitespace/line-breaks so the const POL_RPC_URL and the affected
test declarations/assertions (the blocks around lines referencing POL_RPC_URL
and the multiple test cases) conform to the project's Prettier rules—ensure
consistent indentation, line breaks, and semicolon/quote usage so lint passes.
- Around line 7-16: The test currently falls back to a shared public RPC
(POL_RPC_URL) and runs live mainnet verification, causing flaky CI; change the
setup so POL_RPC_URL is not defaulted to "https://polygon-rpc.com" (remove the
hard fallback) and instead require an explicit environment variable or a test
gate to run network integration (e.g., only use generateProvider with
network:"pol" when RUN_NETWORK_TESTS or POL_RPC is set), and for the non-network
path use a mocked or in-memory provider; also wrap the live verification block
(the test logic around lines 62-80) behind the same gate so CI runs
deterministically.
- Around line 35-40: The test currently only constructs a closure from
verificationBuilder(openAttestationVerifiers, { network: "pol" }) so
getProvider(...) is never executed; update the test to actually invoke the
returned verifier to trigger provider initialization: call const verifier =
verificationBuilder(openAttestationVerifiers, { network: "pol" }); then await
verifier(mockDocument) (or verifier(someMinimalValidDocument)) and assert it
does not throw (or resolves) — ensure the test handles async (use async/await or
return the promise) and provide a minimal mock document suitable for
verification so the provider path is exercised.
In `@test/fixtures/v2/documentPolValidWithToken.ts`:
- Around line 17-18: The fixture contains Prettier formatting issues around the
YAML "version" key and several adjacent scalar lines (the long schema URI
strings) — run Prettier or manually normalize whitespace and quoting so the YAML
scalars conform to the project's Prettier rules (consistent indentation, no
trailing commas, and proper quoting), ensuring the "version" value and the other
multiline/long string entries match the style of other fixtures; after
reformatting, save the file and re-run lint/Prettier to confirm the formatting
errors are resolved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d99866c5-943c-461c-bbf6-ba5dbcc0f79e
📒 Files selected for processing (2)
src/verifiers/documentStatus/tokenRegistry/ethereumPolTokenRegistryStatus.test.tstest/fixtures/v2/documentPolValidWithToken.ts
…istryStatus.test.ts Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…istryStatus.test.ts Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…istryStatus.test.ts Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…istryStatus.test.ts Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…istryStatus.test.ts Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…istryStatus.test.ts Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…istryStatus.test.ts Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…d enhance validation logic - Replaced the v2 document fixture with the v3 version for testing. - Improved test cases to validate fragment statuses and handle W3C VC documents. - Added new tests for various scenarios including missing credential status and invalid document formats. - Removed obsolete v2 document fixture file.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/verifiers/documentStatus/tokenRegistry/ethereumPolTokenRegistryStatus.test.ts (1)
63-85: Remove/replace redundanttest()cases that mutate W3CcredentialStatus(lines 63–85).
openAttestationEthereumTokenRegistryStatus.test()only checks whether the input is an OA wrapped v2/v3 document (and then v2: issuer hastokenRegistry, v3:openAttestationMetadata.proof.method === TokenRegistry). It never reads W3CcredentialStatus, so for the existing W3C VC fixture (already rejected on the earlier test), these mutations can’t influence the result—drop them or switch to OA v2/v3 fixtures to exercise the real missing/incorrecttokenRegistryconditions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/verifiers/documentStatus/tokenRegistry/ethereumPolTokenRegistryStatus.test.ts` around lines 63 - 85, The three redundant tests mutating W3C credentialStatus should be removed or replaced because openAttestationEthereumTokenRegistryStatus.test only inspects OA-wrapped documents; update tests to use OA v2/v3 fixtures instead of the W3C fixture (documentPolValidWithToken) and assert missing/incorrect tokenRegistry by mutating the OA-specific fields: for v2 change issuer.tokenRegistry, for v3 change openAttestationMetadata.proof.method (TokenRegistry) and then call openAttestationEthereumTokenRegistryStatus.test(...) to verify false; remove references to credentialStatus in these cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/verifiers/documentStatus/tokenRegistry/ethereumPolTokenRegistryStatus.test.ts`:
- Line 18: The global jest.setTimeout(300_000) is applied to the whole file
including fast unit tests; move scoping so only the network tests get the long
timeout by removing the top-level jest.setTimeout and placing it inside the
describe block that runs network tests (e.g., add jest.setTimeout(300_000) at
the start of the network-specific describe or a beforeAll within that describe
that calls jest.setTimeout), leaving unit test describes untouched so they use
the default timeout; locate the existing jest.setTimeout call and the
network-related describe that contains the tests which hit the network to apply
this change.
---
Nitpick comments:
In
`@src/verifiers/documentStatus/tokenRegistry/ethereumPolTokenRegistryStatus.test.ts`:
- Around line 63-85: The three redundant tests mutating W3C credentialStatus
should be removed or replaced because
openAttestationEthereumTokenRegistryStatus.test only inspects OA-wrapped
documents; update tests to use OA v2/v3 fixtures instead of the W3C fixture
(documentPolValidWithToken) and assert missing/incorrect tokenRegistry by
mutating the OA-specific fields: for v2 change issuer.tokenRegistry, for v3
change openAttestationMetadata.proof.method (TokenRegistry) and then call
openAttestationEthereumTokenRegistryStatus.test(...) to verify false; remove
references to credentialStatus in these cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 08278100-160b-421f-a8b0-a2fc98e033fa
📒 Files selected for processing (4)
src/verifiers/documentStatus/tokenRegistry/ethereumPolTokenRegistryStatus.test.tstest/fixtures/v2/documentPolValidWithToken.tstest/fixtures/v3/documentPolValidWithToken.jsontest/fixtures/v3/documentPolValidWithToken.ts
…nd improve validation logic - Replaced v3 document fixture with v2 versions for testing. - Enhanced test cases to validate fragment statuses and handle various document scenarios. - Removed obsolete v3 document fixture files. - Updated test logic to ensure proper handling of documents with missing data and issuers.
…istryStatus.test.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…r Amoy token registry - Added .yalc and yalc.lock to .gitignore for local environment configuration. - Updated package.json to reference local dnsprove package. - Updated package-lock.json to include fsevents module. - Introduced new test fixtures for Amoy token registry scenarios, including valid and not issued documents. - Added comprehensive tests for the Amoy network verification logic.
…ant provider bootstrap section
…ration - Eliminated the normalizeNetworkName function call in getDefaultProvider and generateProvider. - Simplified network handling by directly using the provided options or environment variables.
…ompatibility - Deleted the commented-out normalization for "pol" to "matic" in utils.ts, simplifying the codebase.
- Updated the Amoy document verification test to check for a valid hash fragment status instead of just its existence. - Modified the provider configuration in the Polygon network tests to use the jsonrpc provider directly with an explicit RPC URL, enhancing clarity and functionality.
- Changed the default Polygon RPC URL in the test file to use a new endpoint for improved reliability.
…ent handling - Introduced a new JSON fixture for a valid Amoy document with token. - Refactored the TypeScript file to import the document from the new JSON fixture, streamlining the code and improving maintainability. - Updated the Polygon RPC URL in the test configuration for enhanced reliability.
rongquan1
approved these changes
Jun 9, 2026
|
🎉 This PR is included in version 9.7.5 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
What is the background of this pull request?
Changes
Issues
What are the related issues or stories?
Summary by CodeRabbit
Bug Fixes
Tests