Skip to content
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

Initial ZNS 1.1 changes #51

Merged
merged 10 commits into from
Mar 11, 2022
Merged

Initial ZNS 1.1 changes #51

merged 10 commits into from
Mar 11, 2022

Conversation

Remscar
Copy link
Contributor

@Remscar Remscar commented Mar 10, 2022

Preliminary support for ZNS 1.1

@Remscar Remscar changed the title ZNS 1.1 changes Initial ZNS 1.1 changes Mar 10, 2022
Copy link
Contributor

@JamesEarle JamesEarle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly LGTM, some comments and questions

src/helpers/index.ts Show resolved Hide resolved
ipfsGatewayUri: IPFSGatewayUri
): Promise<DomainMetadata> => {
const registrar = await getRegistrarForDomain(hub, domainId);
const metadataUri = await registrar.tokenURI(domainId);
const metadata = await getMetadataFromUri(metadataUri, ipfsGatewayUri);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be a nice idea from now on to do the below for performance, even if this is only a few await statements

const promises = [ ... ] as const;
const [var1, var2, ...] = await Promise.all(promises);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't do that since each promise relies on the last one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't how I missed that, makes sense

src/actions/mintSubdomain.ts Show resolved Hide resolved
src/actions/transferOwnership.ts Show resolved Hide resolved
src/contracts/index.ts Show resolved Hide resolved
@@ -103,13 +104,13 @@ export const createInstance = (config: Config): Instance => {
lockStatus: boolean,
signer: ethers.Signer
): Promise<ethers.ContractTransaction> => {
const registrar: Registrar = await getRegistrar(signer, config.registrar);
const hub: ZNSHub = await getHubContract(signer, config.hub);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose you could simplify this even further by just having config.hub be a parameter to the action, and calling getHubContract inside the action, same for the other functions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what the best pattern will be here yet, I think we'll need to do a refactor pass in the future after the launch.

Making the actions just take a registrar and then doing the domainId->registrar look up outside of them may be wiser.

src/index.ts Outdated Show resolved Hide resolved
src/subgraph/types.ts Show resolved Hide resolved
src/types.ts Show resolved Hide resolved
@Remscar Remscar merged commit 5c1e7b6 into master Mar 11, 2022
@Remscar Remscar deleted the feature/zns1.1 branch May 10, 2022 23:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants