timewave-computer / valence-services

Other
3 stars 0 forks source link

Use bimap for services list on services manager #11

Closed Art3miX closed 1 year ago

Art3miX commented 1 year ago

We have 2 storages that holds service name <-> addr data, check to see if its possible to use bimap for it in CW.

Art3miX commented 1 year ago

@uditvira After discussing this more with @bekauz, we don't see any reason to change the 2 storages system, any change will be more confusing and/or complex in code to understand easily, will greatly increase gas cost, or simply won't give us any gas benefits.

Main reason is that we have 2 usages for this storage:

  1. Admin usage for keeping storage up to date, which is gonna be called rarely if at all.
  2. User usage which is called by a user and is gonna be used a lot.

Optimizing for the first case is pointless as it will be called rarely just to add new services, or if we totally messed something up to update an address for a service, both of those calls are doing 2 save operations and 2 queries, which is not ideal, but the calls are gonna be called couple times a year, and are neglectable.

For the user usage its already optimized, as users only need to perform queries, and both of the available queries are doing a single read operation on a storage, this usage is the more important one as it will be used a lot, and its already use the minimal gas cost possible.

The best way we found to do it, and add as little complexity to it, is by using a single map storage, because we know that service name and address are never gonna be the same string, we can use the string as the key, and abstract away all the operations using a helper storage type (like MAP).

The reasons why we would not want to it is:

  1. This add no benefit to the user usage, and only remove a single query from the admin usage (which is already used rarely).
  2. introducing new storage type for no benefit is pointless
  3. Extra piece of code that needs to be maintained.
Art3miX commented 1 year ago

@uditvira let me know if you think this should be changed, closing for now.