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

Ens changes + module changes #41

Merged
merged 50 commits into from
Feb 15, 2023
Merged

Ens changes + module changes #41

merged 50 commits into from
Feb 15, 2023

Conversation

rahul7668gupta
Copy link
Contributor

@rahul7668gupta rahul7668gupta commented Feb 6, 2023

unit tests after extensive coverage
Screenshot 2023-02-08 at 2 29 01 AM

@rahul7668gupta rahul7668gupta self-assigned this Feb 6, 2023
// get previous resolver
let previousResolverId = domain.resolver;
// create new resolver
let resolverEntity = getOrCreateAirResolver(domain, chainId, resolver);
let resolverEntity = getOrCreateAirResolver(domain, chainId, airBlock, resolver);
Copy link
Member

Choose a reason for hiding this comment

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

are you using airBlock is resolver entity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

air block is being used to create air account, and not being saved inside resolver

let buyerAccount = handleAccountCreation(chainID, NftSales[i].buyer.toHexString(), block.id);
let sellerAccount = handleAccountCreation(chainID, NftSales[i].seller.toHexString(), block.id);
let feeAccount = handleAccountCreation(chainID, NftSales[i].protocolFeesBeneficiary.toHexString(), block.id);
let buyerAccount = getOrCreateAirAccount(chainID, NftSales[i].buyer.toHexString(), block);
Copy link
Member

Choose a reason for hiding this comment

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

Where are you saving these accounts?

Copy link
Contributor Author

@rahul7668gupta rahul7668gupta Feb 8, 2023

Choose a reason for hiding this comment

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

fixing this, by saving this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, rarible subgraph integration, testing and deployment in #43

network: mainnet
source:
abi: Resolver
address: '0x4976fb03C32e5B8cfe2b6cCB31c09Ba78EBaBa41'
Copy link
Member

Choose a reason for hiding this comment

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

Why is this removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is removed coz any domain can have any resolver and we cannot limit to and address, that's why we are listening to all the events of such signature.
that's why we were also doing domain.resolver = resolver.id in resolver events.

Copy link
Contributor

@deepesh-kn deepesh-kn left a comment

Choose a reason for hiding this comment

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

I feel that the code quality should be improved.
We should re evaluate when we should save the entities.

@rahul7668gupta
Copy link
Contributor Author

@deeepesh-kn , requesting re-review for this pr.

Copy link
Contributor

@deepesh-kn deepesh-kn left a comment

Choose a reason for hiding this comment

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

@rahul7668gupta , over all looks good.
I have one comment, please check.

Also please verify that with the changes done to not save the entities in the getOrCreate* function, its handled in the other verticals, and it will not cause any problem there.

@deepesh-kn deepesh-kn merged commit 73691a5 into main Feb 15, 2023
@deepesh-kn deepesh-kn deleted the rahul-ens-changes-3 branch February 15, 2023 09:22
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.

3 participants