Feat/addressprovider ng #20
Conversation
contracts/AddressProviderNG.vy
Outdated
| self.get_id_info[_id].tags = _tags | ||
|
|
||
| # Update metadata (version, update time): | ||
| self._update_entry_metadata(_id) |
There was a problem hiding this comment.
This does not update self.id_tag_mapping so the search by tag will not return the address for the new tags added through this function
There was a problem hiding this comment.
It should also update self.tags with the new tags
| # Emit 0 in version to notify removal of id: | ||
| log EntryModified(_id, 0) | ||
|
|
||
| return True |
There was a problem hiding this comment.
Here too self.id_tag_mapping should be updated (not tragic as the search will return address zero, but still cleaner)
There was a problem hiding this comment.
Ideally would also remove tags from self.tags if that id was last to use them.
contracts/AddressProviderNG.vy
Outdated
| check_id_exists: public(HashMap[uint256, bool]) | ||
| ids: public(DynArray[uint256, 1000]) | ||
| tags: public(DynArray[String[64], 1000]) | ||
| id_tag_mapping: HashMap[bytes32, bool] |
There was a problem hiding this comment.
Is the idea here that an id will not correspond to a specific type of registry anymore as it currently is the case in the AddressProvider? And that instead people should search by tags?
Even if that's not the intended usage, there is a real risk that the link between ID <-> registry type becomes looser. I think that's a slippery slope as it:
- Opens the door to having deprecated registries still on the address provider. This would also make it difficult to tell if a registry is deprecated or not.
- For on-chain integrations, the gas cost of finding the right registry increases with the number of tags and ids. If the integrators can't be sure that an ID corresponds to the reference, most up-to-date version of the registry, they have to use the search.
- I'm not sure tags are great for retrievability. There's no standard taxonomy, casing and spelling might be inconsistent and it will require a human reader. For human-readable info, the description and off-chain docs should be enough.
There was a problem hiding this comment.
You're right in that there's this risk that the link between ID and registry tag becomes looser and it is a slippery slope.
But for now, I see several aggregators who struggle with new factories of stableswap or cryptoswap invariant amm pools. So: if they simply query AddressProviderNG.get_ids_for_tags(_tag), they can get a list of IDs and use that to integrate for off-chain purposes.
the gas cost of finding the right registry increases with the number of tags and ids: I can separate the tags from the AddressInfo struct and that should ameliorate this concern.
Initially I have the following tags: StableSwap, CryptoSwap, LLAMMA, Router, Zap, Factory, Registry, Stablecoin (for ControllerFactory), Lending (for lending ControllerFactory).
contracts/AddressProviderNG.vy
Outdated
| call to `commit_transfer_ownership` | ||
| @return bool success | ||
| """ | ||
| assert msg.sender == self.admin # dev: admin-only function |
There was a problem hiding this comment.
Shouldn't this be called by the new_admin to accept ownership instead?
Forcing the new admin address to accept the transfer for the transfer to go through ensures that the operation doesn't accidentally burn the admin address by making admin a new address that no one controls.
The current setup doesn't seem to really bring any added security
contracts/AddressProviderNG.vy
Outdated
| for _new_tag in _new_tags: | ||
| if not self.check_tag_exists[_new_tag]: | ||
| self.check_tag_exists[_new_tag] = True | ||
| self.tags.append(_new_tag) |
There was a problem hiding this comment.
Do we want remove_tags as well? Typos are common. We might want to clean up deprecated tags at some point too.
contracts/AddressProviderNG.vy
Outdated
|
|
||
|
|
||
| @external | ||
| def remove_id(_id: uint256) -> bool: |
There was a problem hiding this comment.
it would be good to have a corresponding update_id method that lets the admin update all fields at the same time. If an entry is updated, an integrator might expect or need all info to change in the same block. Instead they have to monitor the events for several blocks before they can get the rest of the info, with no guarantee as to what order they're coming in (are tags updated before or after the address?) A utility function that updates all fields at the same time seems more appropriate.
There was a problem hiding this comment.
I think there are two possible improvements that we can add to the new version of the AddressProvider:
The first one is pack IDs and tags together in a uint256:
- encode tags with a one-hot encoding on the 200 MSB of the
uint256:- tags have a unique identifier, a human-readable name can still be associated through a
HashMap(and easily renamed if necessary). - 200 tags are hopefully more than enough but we can squeeze some more tags by reducing the range of the ID.
- no need to hash the tag name with the ID to access
id_tag_mapping, a bitmask suffices to check whether an ID has a tag. - can search for multiple tags very easily.
- can modify multiple tags in a cost-effective manner.
- tags have a unique identifier, a human-readable name can still be associated through a
- encode the ID in the 56 MSB of the
uint256and make it increase sequentially:- the range is more than enough to satisfy all the IDs ever needed.
- I think sequential increase as in the original version is more intuitive for the owner (might be annoying to pick IDs that are not already taken).
The second one (I'm less convinced about) is whether we want to have a "caching system" for addresses grouped by tag.
Since get_addresses_for_tags might be used on-chain as well according to @benber86 it would be nice to have a precomputed list of all the addresses/ids posessing the same tag.
Ideally one would just access a mapping taggedAs[TAG_BITMASK] to obtain the list of all the IDs that have a certain tag.
Every time a new address is assigned one or more new tags, this list needs to be updated. However, this creates additional costs and tricky corner cases (ie, tag deletion). In a sense, it's like deciding whether we want Curve to pay for this (if we use caching) or integrators (if we let them simply use get_addresses_for_tags).
Another option is only use caching for certain tags, known to be often accessed on-chain by integrators.
…etaregistry into feat/addressproviderNG
No description provided.