Hats-Protocol / hats-protocol

Core contracts for Hats Protocol v1.0
https://www.hatsprotocol.xyz
GNU Affero General Public License v3.0
79 stars 25 forks source link

Request and approval flow for tophat links #85

Closed spengrah closed 1 year ago

spengrah commented 1 year ago

This PR modifies the tophat linking functionality to be consentful and prevent spam and social attack vectors. Now, tophats wishing to link to a new tree submit a request, which must be approved by an admin in the new tree before the linkage takes place.

Additionally, requests can now be submitted by either a) the wearer of an unlinked tophat, or b) the admin of an already-linked tophat. This latter option is new, and creates a pathway for parent trees to move child trees to different places within the parent tree.

Also, accounts that are admins of both the linked tree root and the destination admin hat can now "relink" without needing to first go through the request-approval flow.

nintynick commented 1 year ago

@slgraham and I just paired to review this PR. Overall it looks good in the current state, pending some small changes coming shortly.

Documenting some of the considerations we discussed:

  1. As we think about shipping a v1, given the immutability of the contract post-deployment, giving users optionality between two major philosophical approaches to creating Hats trees is something that seemed like it will be beneficial to understand what users what.
  2. The two major philosophical approaches to creating Hats trees as we see them today are: A) creating hats in a single tree, with the benefits of addressable IDs, and B) linking branches that have been permissionlessly created to the official Hats tree for a DAO. (Of course there may be even more approaches than these two.)
  3. Specifically for the linking approach, we want to ensure there is support for i) not breaking the connection between existing hatIDs and token gates, and ii) in the relinking flow, not needing to trust a wearer of a temporarily unlinked tree before reconnecting it to another admin.

Strong work!! 🧢

spengrah commented 1 year ago

pending some small changes coming shortly.

The bulk of these changes are:

  1. Enabling the wearer of the new admin of a linked tophat to approve the linkage request. Previously, this was only possible for admins of the new admin hat, leaving the actual new admin unable to have a say in the matter.

  2. Prevent cross-tree linking, as raised by @gershido. This is accomplished by introducing the concept of a "tippy tophat" --which is the domain of the utmost parent tophat for a given tophat (can also be thought of as the tophat of a tree of trees) -- and ensuring that the to-be-relinked tophat and its would-be new admin share the same tippy tophat, ie are part of the same tree of trees.

gershido commented 1 year ago

All looks good to me except one edge case regarding point (1): _checkAdmin(buildHatId(_newAdminHat, 1)) would not work when _newAdminHat is on the last level

spengrah commented 1 year ago

All looks good to me except one edge case regarding point (1): _checkAdmin(buildHatId(_newAdminHat, 1)) would not work when _newAdminHat is on the last level

Great catch @gershido! Was hoping to not have to do a separate isWearerOfHat check, but I suppose we need to do that for the last level case. Added that in along with associated tests in the most recent commit.