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

Reverse mapping that indexes stream ids by user address #644

Closed
PaulRBerg opened this issue Aug 2, 2023 · 4 comments · Fixed by #679
Closed

Reverse mapping that indexes stream ids by user address #644

PaulRBerg opened this issue Aug 2, 2023 · 4 comments · Fixed by #679

Comments

@PaulRBerg
Copy link
Member

PaulRBerg commented Aug 2, 2023

Task

It may be helpful for all kinds of integration purposes to read all stream ids belonging to specific addresses. In principle, this is how this could be done:

  1. Add a new mapping in LockupLinear and LockupDynamic called _streamsByUser
  2. Implement a new getter getStreamsByUser
  3. When a stream is created, update both the default _streams mapping, as well as the _streamsByUser mapping
  4. Update all tests that check the _streams mapping to also check the _streamsByUser mapping

This feature wouldn't classify streams by status, but it would still be helpful.

Notes

@allwin199
Copy link
Contributor

I would like to work on this, whenever necessary.

@PaulRBerg
Copy link
Member Author

Thanks @allwin199, this is marked as "backlog".

@PaulRBerg
Copy link
Member Author

I have just realized that the task description in the OP was incomplete. _streamsByUser could have been implemented in several ways, that is, user could have represented any of the following:

  1. Just sender
  2. Just recipient
  3. Both sender and recipient

A more precise implementation would have two mappings:

  1. _streamsByRecipient
  2. _streamsBySender

And two getters:

  1. getStreamsByRecipient
  2. getStreamsBySender

@PaulRBerg
Copy link
Member Author

After discussing this feature with @andreivladbrg and @smol-ninja, it has become apparent that this is way more complicated than we had initially expected. To copy-paste Andrei's observations from a private Telegram group:

That's the only thing I can think of for now. The implementation might be complex, though. It would require an algorithm to determine if the streamId exists only in the recipient's array (not for the sender) and to update the mappings accordingly: removing the id from the former recipient and adding it to the new recipient. I wrote a quick implementation to give an idea of how it might look. However, I wouldn't bet on it to be the best/correct way of doing this, since I haven't tested it.

Also, there must be an edge case when the NFT gets burned; both mappings (sender and recipient) should be updated. There are likely other scenarios, but I can't think of them at the moment.

A note about the implementation: I don't believe it's possible to optimize the index search in the array, especially since burns and transfers will disrupt the array's order, preventing it from being strictly ascending.

It looks like we may need to use a library like OpenZeppelin's EnumerableMap. However, implementing it is not trivial. It would take time, and it would increase the gas costs in a significant way.

It is also worth noting that reverse indexes can be implemented off-chain (e.g. via subgraphs).

Given the above, I will tentatively close this issue as not planned.

@PaulRBerg PaulRBerg closed this as not planned Won't fix, can't repro, duplicate, stale Sep 15, 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.

2 participants