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

Create new endpoint for stamp meta data #1235

Closed
1 task
Jkd-eth opened this issue May 5, 2023 · 10 comments · Fixed by #1305 or passportxyz/passport-scorer#248
Closed
1 task

Create new endpoint for stamp meta data #1235

Jkd-eth opened this issue May 5, 2023 · 10 comments · Fixed by #1305 or passportxyz/passport-scorer#248
Assignees

Comments

@Jkd-eth
Copy link
Contributor

Jkd-eth commented May 5, 2023

User Story:

As a developer / integratooor
I want to be able to display the passport stamps a user
So that I can include the stamps within my own dApp such as a user profile

Acceptance Criteria

GIVEN I query a new API endpoint - /registry/stamp-display/{address}
WHEN a wallet has a set of stamps
THEN I receive all stamp metadata to display the stamp including whether the user has the stamp or not

Return the following Meta data by stamp (basically everything in PLATFORMS: PlatformSpec[])

  • icon
  • platform
  • name
  • description
  • connectMessage
    And
  • optional parameter -> whether the user has the stamp or not

Product & Design Links:

Tech Details:

This should be future proof to include new data when we add/update the stamp data

Open Questions:

  • How different is this from /registry/stamps/{address} and do we simply update that endpoint to provide this information?
  • [ ]

Notes/Assumptions:

From Dennison @ Tally (via Telegram)
Basically, if we return a users Stamps, we would love all the metadata of the stamp: Avatar, information about it, callback url, etc…
Because otherwise it’s not really possible for us to show a list of stamps, because we don’t have any context to show the user what they are.
Also t would be nice to be able to show which stamps the user DOES NOT have.

This also relates to #1124

@Jkd-eth Jkd-eth added this to Passport May 5, 2023
@Jkd-eth Jkd-eth moved this to Backlog in Passport May 5, 2023
@nutrina
Copy link
Collaborator

nutrina commented May 9, 2023

@Jeremy-Gitcoin ok, this issue makes sense.
But if we do this, I think it makes sense for us to use the same metadata in Passport, or? This is a better way to guarantee that integrators can have the same experience as Passport users.

@lucianHymer
Copy link
Collaborator

lucianHymer commented May 22, 2023

@nutrina and I discussed this a bit.

We're going to try building the static platform info into a JSON file and serving it out of the /public folder of the Passport repo.

Then we'll provide a scorer API endpoint that fetches, ideally caches, and returns this metadata with an indication of whether a particular address has a stamp or not.

@erichfi The suggested response data doesn't quite line up with our data structure, since we have multiple stamps per platform. I'm assuming we'll return all this data--icon, platform, description, connectMessage--once per platform, and then include a stamps array. Something like

[{
  icon,
  platform,
  description,
  connectMessage,
  stamps: [{
    name,
    claimed
  }]
}]

Let me know if you're looking for something different!

@lucianHymer
Copy link
Collaborator

@erichfi We discussed this further in our engineering meeting.

We're thinking we'll do the following:

On the existing /registry/stamps/{address} endpoint, add an includeMetadata optional parameter that causes the API to return the following with each stamp:

{
  ...,
  metadata: {
    icon,
    platform,
    description,
    connectMessage,
    group,
    name
  }
}

We'll also add an endpoint at /registry/stamp-metadata which will return:

[{
  icon,
  platform,
  description,
  connectMessage,
  groups: [{
    name,
    stamps: ["stamp1", "stamp2"]
  }]
}]

We figure that devs can get data for their UI by either 1. using includeMetadata with the /stamps/{address} endpoint, if they only care about the stamps that a use has or 2. using the data at /stamp-metadata, potentially combining it with the existing data at the /stamps/{address} endpoint, if they want to show info for all possible stamps.

Let me know what you think!

@lucianHymer
Copy link
Collaborator

I've tested this in Postman on review, there's nothing in the UI to review. If we want to solicit feedback on the format of the response for /stamp-metadata and /stamp/address (when include_metadata=true), that's available in the docs here https://api.review.scorer.gitcoin.co/docs

@lucianHymer lucianHymer moved this from In Progress (WIP) to Product/UX Review in Passport May 26, 2023
@0xZakk
Copy link

0xZakk commented May 31, 2023

All of this looks good and definitely works in the way we discussed. One note, which is just something for y'all to consider:

We're structuring the response as an array of objects, which is typical for APIs but does require an added, annoying step for anyone integration: nested looping.

For someone using this metadata to display stamp data in their UI, they're going to first get the list of stamps for a given address (using /registry/stamps/{address}). That endpoint returns an object with an "items" array for each stamp as a sub-object. So to get the provider of a stamp, I'd do something like: res.items[0].credential.provider. If I were to loop through the stamps for an address, I'd do something like:

// stamps is the `items` array in the response from /registry/stamps/{address}
function renderStamps({ stamps }) {
  return stamps.map(stamp => {
    <p>{stamp.credential.provider}</p>
  })
}

Now, to actually use the metadata here, I have to separately loop through the entire metadata response to find the object for the current stamp, for every single stamp an address holds (i.e. nested loop):

// stamps is the `items` array in the response from /registry/stamps/{address}
function renderStamps({ stamps }) {
  return stamps.map(function(stamp) {
    let provider = stamp.credential.provider
    // metadata is the array of objects returned by /registry/stamp-metadata
    // This is the loop (.find()) inside the loop (.map())
    let stampData = metadata.find(metadatum => metadatum.id === provider)

    return (
      <div>
        <h3>{stampData.name}</h3>
        <p>{stampData.description}</p>
      </div>
    )
  })
}

It's maybe not a huge deal, but if we structured the response as an object where the key is the id and matches the provider field in the response from /registry/stamps/{address}, then it becomes a really simple object look-up in my loop:

// stamps is the `items` array in the response from /registry/stamps/{address}
function renderStamps({ stamps }) {
  return stamps.map(stamp => {
    <div>
      <h3>{metadata[stamp.credential.provider].name}</h3>
      <p>{metadata[stamp.credential.provider].description}</p>
    </div>
  })
}

@lucianHymer
Copy link
Collaborator

Hey @0xZakk! Thanks so much for taking a look at this and experimenting with it!

I figured consumers of this data would either:

  1. Care about the stamps a user has, in which case they can use /stamps/{address}, but pass include_metadata=true in order to get the metadata for each stamp. (This would address the use-case in your example, but do consider grouping which I discuss below)
  2. Care about all possible stamps, in which case they can iterate over /stamp-metadata.

Doing anything more complex than either of those base use-cases requires some sort of messing with the data from one or both of these endpoints to get what you want.

It's sort of weird because we're reporting stamps, but the metadata is mostly about the platforms and groups of the stamps. In other words, in your last example, if you care about showing e.g. 1 card per platform, you'd have to collate those stamps by platform (and maybe group, e.g. the "Transactions" group in the ETH platform). It's not super straight forward because of the nested data structure of our stamps.

Does that make sense?

We could still make /stamp-metadata return an object, then people could just Object.values() it if they just want the list like we have now. That would likely make it easier on some integrators. But I wanted to make sure we're aligned on the rest of this!

@0xZakk
Copy link

0xZakk commented May 31, 2023

ah okay! That's great. I don't think I knew about include_metadata=true - is that just a query string I can add to the end of the endpoint?

@lucianHymer
Copy link
Collaborator

lucianHymer commented May 31, 2023

Yeah! It's in the docs under the same old /stamps/{address} endpoint https://api.staging.scorer.gitcoin.co/docs#/Score%20your%20passport/registry_api_v1_get_passport_stamps

@erichfi erichfi moved this from Product/UX Review to Ready to Deploy in Passport Jun 1, 2023
@lucianHymer
Copy link
Collaborator

@0xZakk I went ahead and released this, but I've marked it as beta in the docs. Do you think the requests/responses for /stamp-metadata and /stamps/{address}?include_metadata=true will work as-is? Let me know what you think and I can finalize it and remove the beta warnings in an upcoming release.

Let me know if you want any changes! Do you still think we should switch it over to be an object instead of a list?

@0xZakk
Copy link

0xZakk commented Jun 1, 2023

@lucianHymer thank you for checking! I think this is great

@tim-schultz tim-schultz moved this from Ready to Deploy to Done in Passport Jun 7, 2023
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 a pull request may close this issue.

4 participants