search

decentdao / decent-contracts

Your Safe, Superpowered
https://app.fractalframework.xyz
MIT License
3 stars 3 forks source link

Create top hat #91

Closed adamgall closed 2 months ago

adamgall commented 3 months ago

Closes #92

adamgall commented 2 months ago

Original message posted on June 20 as follows:

The goal of this effort is to allow a single transaction to do two things:

  1. Create a new Top Hat
  2. Allow the Safe to "declare" onchain that "this one and only one Top Hat is mine"

Hats protocol doesn't support "predicting" Top Hat IDs ahead of time, like Safe does with their Safe Proxy contracts. Top Hat IDs are simple auto-incrementing integers.

The concern is that if we try to "predict" the next ID ahead of time in our UI, it's possible that another transaction will be included before ours which creates a new Top Hat, and then all of a sudden the Top Hat we create has an ID that we weren't expecting, and so now the rest of our pre-generated transaction data to "declare" the Top Hat into KeyValuePairs and/or create some actual Hats in that tree, will be referencing a Top Hat that isn't ours.

First thought to overcome that (and confirmed with Hats team) is to make a simple contract that does the Top Hat creation call, gets the ID back, then uses that ID in solidity to do "whatever other onchain things we need to do" with that Top Hat ID.

"Whatever other onchain things we need to do", in our case, includes "declaring the Top Hat ID in KeyValuePairs.

However, KeyValuePairs emits events using msg.sender as the "address" in the "address x is declaring such and such" logs that it emits. As it should!

But that means if we have another address besides the Safe calling KeyValuePairs, that other address shows up as the "from" address in event logs.

So, then I thought, let me delegatecall from the new contract to KeyValuePairs, so that msg.sender inside KeyValuePairs will be the address that called the new contract!

This worked well, EXCEPT for the fact that the log that KeyValuePairs emits isn't emitted from KeyValuePairs any longer! It's emitted "from" the middle contract (CreateTopHatKVPairs) which delegatecalls KeyValuePairs! So that's wild (but expected in retrospect), and doesn't solve our issue because the interface won't see that log as long as it's fetching KeyValuePairs logs from the KeyValuePairs address.

Dead end after dead end. Not sure what the best path forward is.

In this PR are two implementations of the different ways I'm seeing we can approach this problem:

  1. Do the delegatecall to KeyValuePairs anyway, and in the interface we'd need to just look for these ValueUpdated logs on both the KeyValuePairs contract and the new CreateTopHatKVPairs contract.
  2. Don't bother with KeyValuePairs at all, and just emit and look for the log which declares a Top Hat ID from the Safe address in the CreateTopHat contract. The interface would then need to look for logs coming off of this CreateTopHat contract for this data, instead of KeyValuePairs.
adamgall commented 2 months ago

@mudrila i have more code to push into this branch, to support adding admin hat and child hats in this transaction

Also @mudrila I can't really test that because I'm just mocking out my Hats implementation. I don't do anything with those paramaters in my mock function. I mean I guess I could start creating more structure and state in HatsMock, but idk if that's really the point.

adamgall commented 2 months ago

yo @mudrila actually yeah I can't really think of any other tests to write at the moment. Can you?

mudrila commented 2 months ago

@mudrila i have more code to push into this branch, to support adding admin hat and child hats in this transaction

Also @mudrila I can't really test that because I'm just mocking out my Hats implementation. I don't do anything with those paramaters in my mock function. I mean I guess I could start creating more structure and state in HatsMock, but idk if that's really the point.

I feel like trying to make HatsMock as close as possible to the original implementation, at least from minting prospective, will be great - to verify that some sort of integration works here. Can't we not hold our custom mock but use factory for deployment like we're doing for Safe? I guess we can't but just to clarify But otherwise - yeah, I don't see any other reasonable functionality to test.

adamgall commented 2 months ago

Yeah AFAIK there's no quick/easy way to deploy the actual Hats Protocol into our test environment, so that's why I made this MockHats contract.

I think it's not relevant to add anything more to MockHats. DecentHats doesn't care about those properties, it's not our concern. What is our concern is the new topHatId being returned from hats.mintTopHat, cause that needs to get emitted through KeyValuePairs. So Idk, I think what we have tested is fine. Anything else to test will just be testing our ability to implement Hats, but it doesn't seem relevant from the context of what DecentHats cares about.

adamgall commented 2 months ago

Bah there are more tests I can write, i'm just being lazy. let me get to it

adamgall commented 2 months ago

Ok this is ready for review again (I didn't add more tests)

adamgall commented 2 months ago

Closing this PR, the other one will just have all of this new code