Skip to content

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
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.

4 participants