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

Subgraph update #60

Merged
merged 11 commits into from
Dec 22, 2023
Merged

Subgraph update #60

merged 11 commits into from
Dec 22, 2023

Conversation

krisbitney
Copy link
Collaborator

@krisbitney krisbitney commented Dec 15, 2023

This PR adds updates existing entities and adds new entities to the Subgraph:

  • DonorCollective
  • StewardCollective
  • IpfsCollective

These are breaking changes.

The updates will allow us to produce all of the data required by the UI design.

Closes #57

Questions

How do we calculate # of people supported by a donor? As in this part of the donor profile:
Screenshot 2023-12-16 at 11 35 24 AM

what is current pool? As in this part of the ViewCollective page:
Screenshot 2023-12-18 at 12 07 19 PM

which values are guaranteed to exist in a Collective's IPFS metadata?
#57 (comment)

@krisbitney krisbitney requested review from sirpy and removed request for sirpy December 18, 2023 06:41
@krisbitney krisbitney marked this pull request as draft December 18, 2023 07:15
@krisbitney krisbitney marked this pull request as ready for review December 19, 2023 16:33
@@ -95,11 +98,11 @@ type EventData @entity {
rewardPerContributor: BigInt!
contributors: [Steward!]!
nft: ProvableNFT!
claim: Claim
claim: Claim! @derivedFrom(field: "event")
Copy link
Collaborator

Choose a reason for hiding this comment

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

@krisbitney I might miss a discussion / context here
but basically this now reads as:
instead of having many events to 1 claim, there is many claims to 1 event?
Is that right? I looked here and does seem to be the other way around:

for (uint256 i = 0; i < _data.events.length; i++) {

Copy link
Collaborator

Choose a reason for hiding this comment

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

but if this is right, then approved! :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops! I thought it was a 1:1 relationship. I'll get this fixed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is it correct to say the eventUri is unique for each event? If not, can you please advise how we would uniquely identify events?

Alternatively, if events cannot be distinguished with a unique identifier, is there an issue with aggregating the events for each claim so there is one event per claim?

Copy link
Collaborator Author

@krisbitney krisbitney Dec 21, 2023

Choose a reason for hiding this comment

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

When you have a minute, please check the changes in my latest commit. It creates a m:1 relationship for EventData and Claim. It uses eventUri as the unique id for EventData.

@L03TJ3 L03TJ3 self-requested a review December 21, 2023 16:22
@krisbitney krisbitney merged commit cebd745 into web3Integrations Dec 22, 2023
1 check passed
Copy link
Contributor

@sirpy sirpy left a comment

Choose a reason for hiding this comment

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

Still missing subgraph factory to start collecting events for every new pool created

directPaymentPool.stewards = new Array<string>();
directPaymentPool.save();
directPaymentPool.projectId = projectID.toHexString();
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a string I believe, not hex

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is of type Bytes in the event. Are you saying the Bytes represent a utf-8 string?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is fixed in #63

const contributors = event.params.contributers;
const rewardPerContributor = event.params.rewardPerContributer;

const nftAddress = claimId.toHexString();
Copy link
Contributor

Choose a reason for hiding this comment

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

this is incorrect. tokenId is the NFT id in the NFT contract. its not the nft contract address.

Copy link
Contributor

Choose a reason for hiding this comment

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

mostly it should just be renamed to nftId

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

resolved in #63

Comment on lines +11 to +12
donor: Donor! @derivedFrom(field: "collectives")
collective: Collective! @derivedFrom(field: "donors")
Copy link
Contributor

Choose a reason for hiding this comment

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

I dont think this will work. it has to be the other way around.
here it should just be donor: Donor! and collective: Collective!
in Collective and Donor there should be Donors: [DonorCollective!] @derivedFrom(field:"collective")
in Donor Collectives: [DonorCollective!] @derivedFrom(field:"donor")

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wasn't aware that the derivedFrom relationship only works one way. Thanks!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

resolved in #63

Copy link
Contributor

Choose a reason for hiding this comment

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

its not one way, but rather I dont think it can be derived from an array, how would it know which one?

"""NFT's minted to steward"""
nfts: [ProvableNFT!]!
"""Collectives the steward is apart of"""
collectives: [StewardCollective!]!
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as on DonorCollective

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

resolved in #63

Comment on lines +32 to +33
steward: Steward! @derivedFrom(field: "collectives")
collective: Collective! @derivedFrom(field: "stewards")
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as on donorcollective

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

resolved in #63

Comment on lines +43 to +44
donors: [DonorCollective!]!
stewards: [StewardCollective!]!
Copy link
Contributor

Choose a reason for hiding this comment

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

see comment on collectivedonor/steward probably should have derivedfrom field

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

resolved in #63

rewardPerContributor: BigInt!
contributors: [Steward!]!
nft: ProvableNFT!
claim: Claim
claim: Claim! @derivedFrom(field: "events")
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as in CollectiveDonor, probably should be the other way around

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

resolved in #63

@krisbitney
Copy link
Collaborator Author

Still missing subgraph factory to start collecting events for every new pool created

I don't know what this is. Please provide more information.

@sirpy
Copy link
Contributor

sirpy commented Dec 24, 2023

Still missing subgraph factory to start collecting events for every new pool created

I don't know what this is. Please provide more information.

Every pool is a new contract, in order for subgraph to start capturing events for this pool it must be notified about this new contract.
https://thegraph.com/docs/en/developing/creating-a-subgraph/#data-source-templates

@krisbitney
Copy link
Collaborator Author

Still missing subgraph factory to start collecting events for every new pool created

I don't know what this is. Please provide more information.

Every pool is a new contract, in order for subgraph to start capturing events for this pool it must be notified about this new contract. https://thegraph.com/docs/en/developing/creating-a-subgraph/#data-source-templates

Awesome, thanks. This has been fixed in #63

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