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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Emulate Set instead of Array for generic "many values" #96

Merged
merged 2 commits into from Sep 13, 2022

Conversation

ryley-o
Copy link
Contributor

@ryley-o ryley-o commented Sep 12, 2022

Update handleAddManyValueGeneric handler to treat "many values" as a Set instead of an Array.

I think this is desired behavior, but would love input from others on this.

Current behavior:

  • Behave like Array:
    • e.g. If 100 is added twice, generic json would be {<key>: [100, 100]}
      Updated behavior:
  • Behave like Set:
    • e.g. if 100 is added twice, generic json would be: {<key>: [100]}

A Set is typically preferable because how Solidity works so well with mappings, which act more like Sets than Arrays. Emulating a Set in our subgraph eliminates on-chain validation to protect against adding something more than once (reducing gas cost and on-chain complexity). Most relevant current example is this line in our MinterHolder
With this update, smart contract doesn't have to validate if the project is already allowlisted, it simply sets the mapping value to true, and emits an event indicating the project is allowlisted. It is up to the frontend to intelligently avoid duplicate allow/deny requests when queueing up txs.

Cc @mchrupcala since this was based on convo from today 馃檹

@ryley-o ryley-o self-assigned this Sep 12, 2022
@ryley-o ryley-o marked this pull request as ready for review September 12, 2022 23:54
@ryley-o ryley-o requested a review from a team as a code owner September 12, 2022 23:54
@ryley-o ryley-o changed the title Use Set instead of Array for generic "many values" Emulate Set instead of Array for generic "many values" Sep 12, 2022
Copy link
Contributor

@jakerockland jakerockland left a comment

Choose a reason for hiding this comment

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

This tracks for me.

@mchrupcala mchrupcala self-requested a review September 13, 2022 16:00
@ryley-o ryley-o merged commit c76a63f into main Sep 13, 2022
@ryley-o ryley-o deleted the generic-many-as-Set branch September 13, 2022 16:11
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

5 participants