Add indempotency check#263
Merged
ritik4ever merged 3 commits intoJun 29, 2026
Merged
Conversation
- Add `activeServiceExistsByName` to `contractHelpers` in `backend/src/lib/contract.js` - Update `registerServiceOnChain` to prevent duplicate service names for the same provider - Refactor `seed.js` to check for existing services by name before registering, allowing for idempotent re-runs after partial failures - Add unit tests for the new duplicate check logic - Ensure all backend tests pass Co-authored-by: clintjeff2 <119521983+clintjeff2@users.noreply.github.com>
…7529488458 Add seed.js idempotency check per service name
📝 WalkthroughWalkthroughAdds Name-based service deduplication
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (1)
backend/src/lib/contract.test.js (1)
80-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the helper logic instead of only the wrapper.
These specs mock
contractHelpers.activeServiceExistsByName, so they still pass if the helper matches the wrong field or ignoresactive. Please add a direct test that passes pagedfetchServicesdata with active/inactive rows.🤖 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 `@backend/src/lib/contract.test.js` around lines 80 - 113, The current tests for contractLib.activeServiceExistsByName only verify the wrapper with mocked helpers, so they do not prove the helper filters by both name and active status. Add a direct test around activeServiceExistsByName that exercises the real fetchServices path with paged results containing both active and inactive rows, and assert it returns true only when an active service matches the provider and name while ignoring inactive or mismatched entries. Use the existing contractHelpers.activeServiceExistsByName and fetchServices symbols to locate the logic.
🤖 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 `@backend/scripts/seed.js`:
- Around line 43-53: The seed idempotency check in the SERVICE loop is using all
results from listServicesByProvider(), so inactive services are incorrectly
treated as already registered. Update the logic in backend/scripts/seed.js
around existingServices/existingNames to filter for active services only before
building the Set, and keep the skip check in the for-of over SERVICES based on
those active names.
In `@backend/src/lib/contract.js`:
- Around line 424-435: The duplicate-check logic in activeServiceExistsByName
currently matches only provider and name, so inactive rows still block
registration. Update the predicate inside activeServiceExistsByName to also
require s.active when scanning the listServices results, and keep the paging
loop behavior unchanged so only active services prevent registerServiceOnChain.
---
Nitpick comments:
In `@backend/src/lib/contract.test.js`:
- Around line 80-113: The current tests for
contractLib.activeServiceExistsByName only verify the wrapper with mocked
helpers, so they do not prove the helper filters by both name and active status.
Add a direct test around activeServiceExistsByName that exercises the real
fetchServices path with paged results containing both active and inactive rows,
and assert it returns true only when an active service matches the provider and
name while ignoring inactive or mismatched entries. Use the existing
contractHelpers.activeServiceExistsByName and fetchServices symbols to locate
the logic.
🪄 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 Plus
Run ID: a0457f89-0f4f-4b49-81b5-8df15c8285c0
📒 Files selected for processing (3)
backend/scripts/seed.jsbackend/src/lib/contract.jsbackend/src/lib/contract.test.js
Comment on lines
+43
to
+53
| const existingServices = await listServicesByProvider(providerAddress); | ||
| const existingNames = new Set(existingServices.map((s) => s.name)); | ||
|
|
||
| if (count >= SERVICES.length) { | ||
| logger.info('Registry already seeded — skipping'); | ||
| process.exit(0); | ||
| } | ||
| logger.info({ | ||
| total: SERVICES.length, | ||
| existing: existingNames.size, | ||
| }, 'Starting seed idempotency check'); | ||
|
|
||
| for (const svc of SERVICES) { | ||
| if (existingNames.has(svc.name)) { | ||
| logger.info({ name: svc.name }, 'Service already registered — skipping'); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Only skip names that are still active.
listServicesByProvider() includes inactive services. Building existingNames from every row means a previously deactivated service will be treated as already registered and never retried.
Suggested fix
- const existingNames = new Set(existingServices.map((s) => s.name));
+ const existingNames = new Set(
+ existingServices.filter((s) => s.active).map((s) => s.name)
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const existingServices = await listServicesByProvider(providerAddress); | |
| const existingNames = new Set(existingServices.map((s) => s.name)); | |
| if (count >= SERVICES.length) { | |
| logger.info('Registry already seeded — skipping'); | |
| process.exit(0); | |
| } | |
| logger.info({ | |
| total: SERVICES.length, | |
| existing: existingNames.size, | |
| }, 'Starting seed idempotency check'); | |
| for (const svc of SERVICES) { | |
| if (existingNames.has(svc.name)) { | |
| logger.info({ name: svc.name }, 'Service already registered — skipping'); | |
| const existingServices = await listServicesByProvider(providerAddress); | |
| const existingNames = new Set( | |
| existingServices.filter((s) => s.active).map((s) => s.name) | |
| ); | |
| logger.info({ | |
| total: SERVICES.length, | |
| existing: existingNames.size, | |
| }, 'Starting seed idempotency check'); | |
| for (const svc of SERVICES) { | |
| if (existingNames.has(svc.name)) { | |
| logger.info({ name: svc.name }, 'Service already registered — skipping'); |
🤖 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 `@backend/scripts/seed.js` around lines 43 - 53, The seed idempotency check in
the SERVICE loop is using all results from listServicesByProvider(), so inactive
services are incorrectly treated as already registered. Update the logic in
backend/scripts/seed.js around existingServices/existingNames to filter for
active services only before building the Set, and keep the skip check in the
for-of over SERVICES based on those active names.
Comment on lines
+424
to
+435
| activeServiceExistsByName: async function (provider, name, fetchServices = listServices) { | ||
| let page = 0; | ||
| const pageSize = 20; | ||
|
|
||
| while (true) { | ||
| const services = await fetchServices({ page, pageSize }); | ||
| if (!services.length) { | ||
| return false; | ||
| } | ||
|
|
||
| if (services.some((s) => s.provider === provider && s.name === name)) { | ||
| return true; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Honor the active flag in this duplicate check.
listServices() returns inactive rows too, but this predicate ignores s.active. A deactivated service with the same provider/name will still block registerServiceOnChain, which contradicts the new "duplicate active service" behavior.
Suggested fix
- if (services.some((s) => s.provider === provider && s.name === name)) {
+ if (services.some((s) => s.active && s.provider === provider && s.name === name)) {
return true;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| activeServiceExistsByName: async function (provider, name, fetchServices = listServices) { | |
| let page = 0; | |
| const pageSize = 20; | |
| while (true) { | |
| const services = await fetchServices({ page, pageSize }); | |
| if (!services.length) { | |
| return false; | |
| } | |
| if (services.some((s) => s.provider === provider && s.name === name)) { | |
| return true; | |
| activeServiceExistsByName: async function (provider, name, fetchServices = listServices) { | |
| let page = 0; | |
| const pageSize = 20; | |
| while (true) { | |
| const services = await fetchServices({ page, pageSize }); | |
| if (!services.length) { | |
| return false; | |
| } | |
| if (services.some((s) => s.active && s.provider === provider && s.name === name)) { | |
| return true; | |
| } |
🤖 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 `@backend/src/lib/contract.js` around lines 424 - 435, The duplicate-check
logic in activeServiceExistsByName currently matches only provider and name, so
inactive rows still block registration. Update the predicate inside
activeServiceExistsByName to also require s.active when scanning the
listServices results, and keep the paging loop behavior unchanged so only active
services prevent registerServiceOnChain.
Contributor
Author
|
@ritik4ever , please review and merge. Assign more issues and Allow a rating for me as well please. |
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.
This PR improves the idempotency of the
seed.jsscript. Previously, the script would skip seeding entirely if the total count of services was greater than or equal to the number of services in the seed list. This caused issues when the script partially failed, as subsequent runs would not attempt to register the remaining services.The new implementation fetches all services registered by the provider and checks each service in the seed list by name. It only attempts to register services that are not already present on-chain. Additionally,
registerServiceOnChainnow explicitly checks for name collisions to ensure on-chain consistency.Changes:
activeServiceExistsByNamehelper tobackend/src/lib/contract.js.registerServiceOnChaininbackend/src/lib/contract.jsto throw an error if a service with the same name already exists for the provider.backend/scripts/seed.jsto use name-based idempotency.backend/src/lib/contract.test.jsto verify name-based duplicate detection.Closes #75
Summary by CodeRabbit
New Features
Bug Fixes