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

Implement Cached Score Retrieval in Gitcoin Passport Smart Contracts #1924

Closed
nutrina opened this issue Nov 20, 2023 · 6 comments · Fixed by passportxyz/eas-proxy#45
Closed
Assignees

Comments

@nutrina
Copy link
Collaborator

nutrina commented Nov 20, 2023

User Story:

As a passport developer and integrator,
I want a gas-efficient method, like a getCachedScore function within the resolver,
To retrieve the numeric score of a user’s Passport from a cached mapping, thus ensuring minimal gas costs during operations like gating smart contract calls.

Acceptance Criteria

GIVEN I am a developer integrating with the Gitcoin Passport system,
WHEN I call the getCachedScore(address user) function of the GitcoinResolver smart contract,
THEN I expect the cached Score structure for the user's Passport to be returned from a cached mapping within the resolver smart contract,
AND the function should revert the transaction if no score is available for the provided ETH address.

Product & Design Links:

Not applicable.

Tech Details:

  • Introduce a cached mapping, such as mapping (address => Score), in the GitcoinResolver smart contract where:
// The following struct should ideally fit into a 32 byte storage slot
struct Score {
  uint32 score;           // considering only 4 decimals should be sufficient
  uint64 issuanceDate;    // For checking the age of the stamp, without loading the attestation
  uint64 expirationDate;  // This makes sense because we want to make sure the stamp is not expired, and also do not want to load the attestation 
  uint32 scorerId;        // would we need this ???
}
  • This caching strategy aims to bypass the costlier process of loading entire EAS (Ethereum Attestation Service) attestations.
  • Ensure that the cache is updated when attesting & revoking attestations:
    • save the value on the attest hook
    • delete the value on the revoke hook

Open Questions:

Not applicable.

Notes/Assumptions:

  • Assumes that the primary goal is to optimize gas costs while maintaining the functionality and reliability of the score retrieval process in smart contracts.
  • Assumes that the caching mechanism will align with the existing infrastructure of the Gitcoin Passport system.
@nutrina nutrina added this to Passport Nov 20, 2023
@nutrina nutrina converted this from a draft issue Nov 20, 2023
@erichfi erichfi changed the title Caching score Implement Cached Score Retrieval in Gitcoin Passport Smart Contracts Nov 20, 2023
@nutrina
Copy link
Collaborator Author

nutrina commented Nov 20, 2023

To be discussed further: we could also change the number of digits in the value that we store in the EAS attestations.
This will make any transformation / casting of the value to fie in the score attribute of Score structure specified above easier. (ideally a simple cast suffices).
@lebraat

@tim-schultz tim-schultz self-assigned this Nov 21, 2023
@nutrina nutrina moved this from Prioritized to In Progress (WIP) in Passport Nov 22, 2023
@nutrina nutrina self-assigned this Nov 22, 2023
@nutrina nutrina moved this from In Progress (WIP) to Blocked in Passport Nov 23, 2023
@nutrina
Copy link
Collaborator Author

nutrina commented Nov 23, 2023

Waiting to review this with somebody.

@tim-schultz tim-schultz moved this from Blocked to In Progress (WIP) in Passport Nov 27, 2023
@tim-schultz
Copy link
Collaborator

tim-schultz commented Nov 27, 2023

This looks good to me. Imo we should wait on deploying this until #1928 is also deployed it seems like it could introduce some confusion about the score value if not deployed together.

@tim-schultz tim-schultz moved this from In Progress (WIP) to Blocked in Passport Nov 27, 2023
@nutrina nutrina moved this from Blocked to In Progress (WIP) in Passport Nov 28, 2023
@nutrina nutrina moved this from In Progress (WIP) to Blocked in Passport Nov 28, 2023
@tim-schultz tim-schultz moved this from Blocked to In Progress (WIP) in Passport Nov 29, 2023
@tim-schultz tim-schultz moved this from In Progress (WIP) to Product/UX Review in Passport Nov 29, 2023
@tim-schultz
Copy link
Collaborator

These changes have been merged let us know if we should deploy this before #1928 is completed

@nutrina nutrina moved this from Product/UX Review to Prioritized in Passport Dec 4, 2023
@nutrina nutrina moved this from Prioritized to In Progress (WIP) in Passport Dec 4, 2023
@nutrina
Copy link
Collaborator Author

nutrina commented Dec 4, 2023

Putting these back in progress as I have found some bugs:

  • we don't account for Scores written as part of a multiAttest (have already created a failing test for this)
  • we do not delete a cached score when revoking

@nutrina nutrina reopened this Dec 4, 2023
@nutrina nutrina moved this from In Progress (WIP) to Product/UX Review in Passport Dec 7, 2023
@nutrina
Copy link
Collaborator Author

nutrina commented Dec 7, 2023

The work on these branches has been merged int the branch for #1928

@erichfi erichfi moved this from Product/UX Review to Ready to Deploy in Passport Dec 11, 2023
@erichfi erichfi closed this as completed Dec 18, 2023
@erichfi erichfi moved this from Ready to Deploy to Done in Passport Dec 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
3 participants