feat: add hasVerifiedAttestations and getVerifiedAttestations #95

alvaroraminelli · 2024-02-07T05:08:07Z

What changed? Why?

Introduce new functionalities that make easier verify addresses in the Ethereum Attestation Service (EAS).

hasVerifiedAttestations
getAttestation

Notes to reviewers

How has it been tested?

Local build and consuming in a playground

alvaroraminelli · 2024-02-07T16:57:34Z

Missing tests

cbfyi · 2024-02-07T17:29:32Z

src/core/getAttestation.ts

+`;
+
+/**
+ * Retrieves attestations for a given address and chain, optionally filtered by schemas.


This is in the context of Coinbase Verifications. Do we want to be more specific here in that case?

e.g. getCoinbaseVerifications

Same with below, should we name it hasCoinbaseVerifications ?

Bringing this up in case we extend Onchain Kit to support all attestations in general -- which IMO, we should!

what is the path to support all attestations in general?

I don't think we ever settled on a clear path for this.

But absent any decisions, I think we should be more opinionated with the naming to leave room for more general EAS support in the near future. Committing to something abstract like getAttestation which really only targets CB Verifications would preclude us from that option.

cc: @Zizzamia

cbfyi · 2024-02-07T17:31:49Z

src/core/getAttestation.ts

+ * @returns A promise that resolves to an array of Attestations.
+ * @throws Will throw an error if the request to the GraphQL API fails.
+ */
+export async function getAttestation(


Did we set this up manually?

I was thinking longer term we could just use https://the-guild.dev/graphql/codegen

Allows us to appropriately keep up with EAS' GraphQL specs, and also more easily add new helpers.

What do we want to codegen? I am generally against codegen as they are the eveil of good developer experience for OSS libraries.

Basically this.

If you look at the link I shared, you can arguably get the same experience, without having to manually plumb the queries.

In another scenario I'd agree with you if we're actually adding significantly more value / logic over what codegen already provides.

What kind of codegen we want? Like one for each Attestation? help me understand

Depends. In this context, we are asking GraphQL for "attestations that are not expired or revoked for a specific address, a specific attester and possibly a set of schema IDs". There's an additional dimension here which is the network.

I'd personally codegen what we already have:

query findActiveAttestations(...) { // ... current fields, but broken out as a fragment ... }

Then this current function or if we rename it to findCoinbaseVerifications / getCoinbaseVerifications will just be a light wrapper around the generated code. But this ensures strong consistency with the actual EAS GraphQL schema, and we're not writing our types ourselves making it easier to maintain longer term. Our focus will simply be providing a better experience over someone writing their own queries or generating their own GraphQL client.

Then for the other use-case in this context, we have hasVerifiedAttestations which can simply be another codegen query because there's actually a specific GraphQL resolver for that! That is, findFirstAttestation in EAS' GraphQL. This way, we are letting the GraphQL API handle most of the filtering for us.

BTW, I'm not strongly holding this opinion. It's something we can do later. I rather we don't block this PR from going out. But it would address your other comment:

Would love we have strong types on what's returing and what each of them means.

In the sense that we would have the types auto-gen.

cbfyi · 2024-02-07T17:33:23Z

src/core/getAttestation.ts

+ * @returns A promise that resolves to an array of Attestations.
+ * @throws Will throw an error if the request to the GraphQL API fails.
+ */
+export async function getAttestation(


nit: This implicitly filters out revoked or expired attestations. We may want a different name for it to reflect that. Similar to comment further up.

src/core/getAttestation.ts

cbfyi · 2024-02-07T17:43:30Z

src/core/hasVerifiedAttestations.ts

+  const schemaUids = getChainSchemasUids(expectedSchemas, chain.id);
+  const attestations = await getAttestation(chain, address, { schemas: expectedSchemas });
+  const schemasFound = attestations.map((attestation) => attestation.schemaId);
+
+  return schemaUids.every((schemaUid) => schemasFound.includes(schemaUid));


nit: no change needed, but a more optimal solution would be to let the GraphQL server handle all this.

README.md

Zizzamia · 2024-02-07T19:16:28Z

package.json

@@ -18,6 +18,8 @@
    "release:version": "changeset version && yarn install --immutable"
  },
  "peerDependencies": {
+    "graphql": "^16.8.1",


Will this clash with any other GraphQL dependcy in the industry? I am a bit ingronant here, but those work this well with Relay?

wondering if we can say

"graphql": "*",

or choose a minium version.

Not an expert here, who should we loop in on this?

graphql is the base dependency which Relay relies on too. But as we are setting this as a peer dependency, we're less likely to have clashes.

src/core/attestation.ts

src/core/getAttestation.ts

Zizzamia · 2024-02-07T22:33:35Z

src/core/types.ts

+  schemaId: string;
+  timeCreated: number;
+  txid: string;
+};


Can we add a line of comment for each property, so we know what they are used for.

Zizzamia · 2024-02-07T22:35:10Z

src/core/hasVerifiedAttestations.ts

+export async function hasVerifiedAttestations(
+  chain: Chain,
+  address: Address,
+  expectedSchemas: AttestationSchema[] = [],


expectedSchemas sounds odd. What exactly are we passing here?

Oh I see we have those 'VERIFIED ACCOUNT' | 'VERIFIED COUNTRY', not sure I follow.

Step 1, tell me the chain
Step 2, tell me an address, ok but who address? Customer address? unclear.
Step 3, tell me a schema, ok what schema? how do I know where those schemas are?

do we want to be more opinionated on naming address? what if it is not a customer, it is something else to me. Might name like 'walletAddress'. But I assume that address in onchain app is clear.

address is probably fine, but we need good example in the docs on how this works and how and when should be used.

I think address is fine. Otherwise it would be target / recipient / subject, all of which may not convey the desired meaning.

Zizzamia · 2024-02-08T07:45:25Z

src/core/getAttestation.ts

+export async function getAttestation<TChain extends Chain>(
+  address: Address,
+  chain: TChain,
+  filters?: { schemas?: AttestationSchema[] },


@cbfyi curios, what other kind of filters we imagine to have later on?

I guess GraphQL filters maybe

where: { AND: [conditions] }, orderBy: [{ timeCreated: 'desc' }], distinct: ['schemaId', 'attester'], take: 10,

mmmm

Yeah perhaps we can drive this with use-cases:

Gating: I want to know if someone has X, Y, Z attestation

Display / Visual: I want to get all latest attestations to display, by attester and/or schema

Additional Logic: I want to get a specific attestation to check its payload / contents

And for both (1), and (2), we would want to filter by only active by default. (3) doesn't even have to hit GraphQL, but if we want the payload / contents to be unpacked, it's more convenient if we do.

That should cover 70% of apps leveraging attestations IMO.

alvaroraminelli · 2024-02-13T17:24:43Z

closing as going to break down in smaller PRs.

alvaroraminelli added 2 commits February 6, 2024 21:07

feat: add hasVerifiedAttestations and getVerifiedAttestations

eba43e3

feat: refactor attestation methods

ba1a847

alvaroraminelli requested a review from Zizzamia February 7, 2024 16:47

fix: format

d2abdea

alvaroraminelli marked this pull request as ready for review February 7, 2024 16:57

cbfyi reviewed Feb 7, 2024

View reviewed changes

src/core/getAttestation.ts Outdated Show resolved Hide resolved

cbfyi reviewed Feb 7, 2024

View reviewed changes

src/core/getAttestation.ts Show resolved Hide resolved

cbfyi reviewed Feb 7, 2024

View reviewed changes