-
Notifications
You must be signed in to change notification settings - Fork 4
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
Conversation
domain module refactor + air extra
// get previous resolver | ||
let previousResolverId = domain.resolver; | ||
// create new resolver | ||
let resolverEntity = getOrCreateAirResolver(domain, chainId, resolver); | ||
let resolverEntity = getOrCreateAirResolver(domain, chainId, airBlock, resolver); |
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.
are you using airBlock is resolver entity?
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.
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); |
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.
Where are you saving these accounts?
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.
fixing this, by saving this
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.
done, rarible subgraph integration, testing and deployment in #43
network: mainnet | ||
source: | ||
abi: Resolver | ||
address: '0x4976fb03C32e5B8cfe2b6cCB31c09Ba78EBaBa41' |
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.
Why is this removed?
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 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.
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.
I feel that the code quality should be improved.
We should re evaluate when we should save the entities.
@deeepesh-kn , requesting re-review for this pr. |
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.
@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.
unit tests after extensive coverage
