-
Notifications
You must be signed in to change notification settings - Fork 2
[v1.0] Domain data aggregation and validation for use in the migration #133
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
[v1.0] Domain data aggregation and validation for use in the migration
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs some updates and please address the comments.
This should also be updated from the base branch after bulk registration PR is merged.
@JamesEarle make sure you update the document here with the exact Execution section that outlines full process step by step! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR sets up domain data aggregation from a subgraph, validates it against onchain data, and scaffolds the first migration steps for registering domains in bulk.
- Introduces tools to fetch and cache ZNS contracts (
zns-contract-data.ts
) - Implements
validateDomain
to compare subgraph vs onchain state - Adds scripts for subgraph pagination, validation (
01_validation.ts
), and bulk registration (02_registration.ts
)
Reviewed Changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
test/helpers/types.ts | Made zeroVaultAddress optional to reflect missing field scenarios |
src/utils/migration/zns-contract-data.ts | Added getZNS to load and cache ZNS contract instances from DB |
src/utils/migration/validate.ts | Implemented validateDomain to assert subgraph vs onchain consistency |
src/utils/migration/types.ts | Defined Domain , config, and migration-related TypeScript interfaces |
src/utils/migration/subgraph/queries.ts | Created GraphQL query for users and domains |
src/utils/migration/subgraph/client.ts | Provided Apollo client factory with env-based subgraph URL |
src/utils/migration/subgraph/index.ts | Implements pagination logic to fetch all users and domains |
src/utils/migration/database.ts | MongoDB adapter and getZNSFromDB to retrieve onchain contract data |
src/utils/migration/01_validation.ts | Script to validate domains and store valid sets to MongoDB |
src/utils/migration/02_registration.ts | Script to register validated domains in bulk onchain |
src/utils/migration/registration.ts | Core logic for batching and executing domain registration transactions |
hardhat.config.ts | Updated network accounts for read-only mainnet operations |
Files not reviewed (1)
- package.json: Language not supported
Comments suppressed due to low confidence (3)
src/utils/migration/validate.ts:83
- The message string is outside the
assert.equal
call due to misplaced parentheses; it won't be displayed on failure. Move the backtick message into theassert.equal
arguments.
assert.equal(domain.parentHash, ZeroHash), `Domain ${domain.id} 'isWorld' is true, but has parent hash`;
src/utils/migration/subgraph/queries.ts:39
- The field
address
appears twice in this query (line 23 and here), which will cause a GraphQL error. Remove the duplicate.
address
src/utils/migration/registration.ts:52
- There's an extra
}
at the end of the template literal, causing a malformed log message. Remove the surplus brace.
console.log(`Skipping already registered domains: ${i} to ${i + sliceSize}}`);
// TODO figure out error with users who have not called to reclaim there domain | ||
// after a transfer | ||
for (const domain of domains) { | ||
const error = await validateDomain(domain, zns, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
validateDomain
only accepts two arguments (domain
and zns
), but three are provided here. Remove the extra argument to match the signature.
const error = await validateDomain(domain, zns, true); | |
const error = await validateDomain(domain, zns); |
Copilot uses AI. Check for mistakes.
sliceSize : number, | ||
start : number, | ||
) => { | ||
const registeredDomains = Array<RegisteredDomains>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Using Array<RegisteredDomains>()
to initialize an array is unconventional and may confuse readers. Consider using const registeredDomains: RegisteredDomains[] = []
for clarity.
const registeredDomains = Array<RegisteredDomains>(); | |
const registeredDomains: RegisteredDomains[] = []; |
Copilot uses AI. Check for mistakes.
has conflicts. needs to be updated from the base branch |
…alidation, registration needs rework after fundamental other changes.
Please, update from base branch! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a couple little comments and needs an update from base. after that - good to go!
…egistration is in comments but will be implemented after larger merge of multiple PRs. Run linter
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## support/v1.0 #133 +/- ##
=============================================
Coverage 99.80% 99.80%
=============================================
Files 11 11
Lines 509 509
Branches 126 126
=============================================
Hits 508 508
Misses 1 1 🚀 New features to boost your workflow:
|
Grab data from subgraph and validate against onchain data through Ethers.
The migration hasn't been defined but all solutions will use this as a first step. The registration script hasn't been altered since it's original inception as a result as it may be usable later.