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

Add GitPoap & Degenscore Data Provider #1555

Merged
merged 24 commits into from
Mar 18, 2023

Conversation

markus0x1
Copy link
Contributor

@markus0x1 markus0x1 commented Mar 17, 2023

fixes

#1430
#1432

@markus0x1 markus0x1 changed the title Add GitPoap Add GitPoap & Degenscore Mar 17, 2023
@markus0x1 markus0x1 changed the title Add GitPoap & Degenscore Add GitPoap & Degenscore Data Provider Mar 17, 2023
@MartinGbz
Copy link
Contributor

MartinGbz commented Mar 17, 2023

Hi guys @markuschick @3Vis3 @mme022 , please ping me when you need a review, thanks! :)

@markus0x1
Copy link
Contributor Author

Hey @MartinGbz 👋
Feel free to review

return Object(holders).keys().length;
}

public async getGitPoapEventsIDByAddress(address: string ): Promise<FetchedData> {
Copy link
Contributor

Choose a reason for hiding this comment

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

the function here is not really use full because it fetch all the gitpoap ids for an address and returns an empty FetchData object.
And you can't make a group with gitpoap if, you can only create group with web2 or web3 accounts.
So I don't see the utility of this function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed with 39b7462

@MartinGbz
Copy link
Contributor

@markuschick hey, so it's seems pretty gooof, I just add a comment.
Is it possible for you to give me a JSON_RPC_URL in order for me to test it and then also add it to our infra in order for anyone to use your data provider? Thanks!

@MartinGbz
Copy link
Contributor

MartinGbz commented Mar 18, 2023

@markuschick if you could now send me the JSON_RPC_URL in DM on discord (martingbz.eth#3809) or telegram (martingbz) or twitter (0xMartinGbz), that would be perfect :)

@markus0x1
Copy link
Contributor Author

@markuschick hey, so it's seems pretty gooof, I just add a comment. Is it possible for you to give me a JSON_RPC_URL in order for me to test it and then also add it to our infra in order for anyone to use your data provider? Thanks!

Added Json RPC dcf874c

@MartinGbz MartinGbz changed the base branch from main to gitpoap-degenscore-dp-merge March 18, 2023 09:38
Copy link
Contributor

@MartinGbz MartinGbz left a comment

Choose a reason for hiding this comment

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

@markuschick made a review
There is a few things to change

@MartinGbz
Copy link
Contributor

MartinGbz commented Mar 18, 2023

Resolve each review when you fixed them, and ping me when you have finished, thanks 😊

Copy link
Contributor

@0xmme 0xmme left a comment

Choose a reason for hiding this comment

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

we added the suggested changes, thanks for the review!

@markus0x1
Copy link
Contributor Author

we added the suggested changes, thanks for the review!

@MartinGbz

@MartinGbz
Copy link
Contributor

MartinGbz commented Mar 18, 2023

we added the suggested changes, thanks for the review!

I already added a new review ;) @markuschick @mme022

);
}

public async getBeaconOwnersWithScore(args: { _score: number }) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@markuschick here you can only pass this: { _score }: Score

And create a score type like this: export type Score = { _score: number }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed with fixed with 61f7d4c

return dataProfiles;
}

public async getGitPoapHoldersByEventIdCount(getGitPoapHoldersArg: {gitPoapEventId: string} ): Promise<number> {
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed with 61f7d4c


generate: async (context: GenerationContext): Promise<GroupWithData[]> => {
const degenscoreProvider = new dataProviders.DegenScoreProvider();
const addresses = await degenscoreProvider.getBeaconOwnersWithScore(900);
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to change this part of your group, getBeaconOwnersWithScore require a type Score

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed with b3ba22f

@MartinGbz
Copy link
Contributor

@markuschick @mme022 I added a last review regarding the group

@MartinGbz MartinGbz merged commit a0d9aeb into sismo-core:gitpoap-degenscore-dp-merge Mar 18, 2023
MartinGbz added a commit that referenced this pull request Mar 20, 2023
* Add GitPoap & Degenscore Data Provider (#1555)

Co-authored-by: mme <9083787+mme022@users.noreply.github.com>
Co-authored-by: 3Vis <francescotrevisan3@gmail.com>
Co-authored-by: 3Vis <31535611+3Vis3@users.noreply.github.com>

* import groups to index

* fix: gitpoap + degenscore providers

* fix: degenscore provider

---------

Co-authored-by: markus <33206087+MarkuSchick@users.noreply.github.com>
Co-authored-by: mme <9083787+mme022@users.noreply.github.com>
Co-authored-by: 3Vis <francescotrevisan3@gmail.com>
Co-authored-by: 3Vis <31535611+3Vis3@users.noreply.github.com>
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

4 participants