Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: add hasVerifiedAttestations and getVerifiedAttestations #95
feat: add hasVerifiedAttestations and getVerifiedAttestations #95
Changes from all commits
eba43e3
ba1a847
d2abdea
0f1794f
aaf4eb5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 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
?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.
Bringing this up in case we extend Onchain Kit to support all attestations in general -- which IMO, we should!
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.
what is the path to support all attestations in general?
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 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
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.
@cbfyi curios, what other kind of filters we imagine to have later on?
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 guess GraphQL filters maybe
mmmm
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.
Yeah perhaps we can drive this with use-cases:
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.
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.
expectedSchemas
sounds odd. What exactly are we passing here?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.
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?
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.
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.
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.
address
is probably fine, but we need good example in the docs on how this works and how and when should be used.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 think
address
is fine. Otherwise it would betarget
/recipient
/subject
, all of which may not convey the desired meaning.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.
Can we add a line of comment for each property, so we know what they are used for.