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

New Feature Termed Roles #102

Merged
merged 124 commits into from
Oct 30, 2024
Merged

Conversation

Da-Colon
Copy link
Contributor

@Da-Colon Da-Colon commented Sep 17, 2024

Adds DecentAutonomousAdmin Contract

  • Used for trigging next start and flush any sablier funds to previous elected wearer

Adds DecentHats_0_2_0 Contract

  • DecentAutoAdmin clone is deployed and added as wearer of Admin Hat
  • Updated to create Termed Roles when prompted
  • This involves making the deploying a instance of Hats Election Module and setting the eligibility module

Closes #103
Closes #104
Closes #105

@Da-Colon Da-Colon changed the title Add contract/decent autonomous admin hat Add contract | DecentAutonomousAdminHat Sep 17, 2024
@Da-Colon Da-Colon self-assigned this Sep 26, 2024
contracts/interfaces/IDecentAutonomousAdmin.sol Outdated Show resolved Hide resolved
contracts/mocks/MockHats.sol Outdated Show resolved Hide resolved
contracts/mocks/MockHatsAdmin.sol Outdated Show resolved Hide resolved
contracts/mocks/MockHatsAdmin.sol Outdated Show resolved Hide resolved
contracts/DecentAutonomousAdmin.sol Outdated Show resolved Hide resolved
contracts/DecentAutonomousAdmin.sol Outdated Show resolved Hide resolved
contracts/DecentHats.sol Outdated Show resolved Hide resolved
contracts/DecentHats.sol Outdated Show resolved Hide resolved
contracts/DecentHats.sol Outdated Show resolved Hide resolved
contracts/DecentHats.sol Outdated Show resolved Hide resolved
Comment on lines 369 to 380
uint256 hatId = _createHat(
hatsProtocol,
adminHatId,
hat,
topHatAccount,
eligibilityAddress
);

IHatsElectionEligibility(eligibilityAddress).elect(
hat.termedParams[0].termEndDateTs,
hat.termedParams[0].nominatedWearers
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick, flip the order of these two statements.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it doesn't really matter, but it seem to make sense to create the hat then elect?

Copy link
Member

@adamgall adamgall Oct 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeahh... my nitpick is just because the hatId variable isn't used in the elect call, but it's used in the line below that. My intention with the nitpick comment was just to keep relevant code close to each other.

I guess it doesn't really matter, but it seem to make sense to create the hat then elect?

but the election doesn't depend on the new hatId, is what i'm saying.

contracts/DecentHats.sol Outdated Show resolved Hide resolved
contracts/DecentHats.sol Outdated Show resolved Hide resolved
contracts/DecentHats.sol Outdated Show resolved Hide resolved
Comment on lines 180 to 195
function createRoleHat(CreateRoleHatParams calldata params) external {
_createHatAndAccountAndMintAndStreams(
params.hatsProtocol,
params.registry,
params.topHatAccount,
params.hatsAccountImplementation,
params.adminHatId,
params.hat
);

params.hatsProtocol.transferHat(
params.topHatId,
address(this),
msg.sender
);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the last thing to do in this contract, is clean up this standalone function.

@DarksightKellar this one is on you.

So, the reason why we need to transfer the top back back to the safe, is because this contract is the current wearer of the top hat by the time this function is being called. We did that (transaction was created in the frontend and part of the bundle of txs in the proposal that this call is in) because this contract can't create a new hat on a tree unless it's an admin of the hat it's trying to create. Makes sense.

BUT, instead of transferring the top hat to this contract, why not utilize the fact that we've added this contract as a module on the Safe! As of right now, adding this contract as a module on the Safe (in the context of minting a new hat via this call), is completely unnecessary, unless we actually call execTransactionFromModule in this function call.

So let's do that instead of transferring the top hat around.

Please proxy the call to create a new hat through the Safe via execTransactionFromModule, and remove the transferHat instruction. In the frontend, no need to transfer the top hat to this contract.

contracts/DecentHats.sol Outdated Show resolved Hide resolved
Copy link
Member

@adamgall adamgall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last set of change requests, related to the two external standalone functions which create new Hats on existing trees.

Those functions shouldn't need to transfer the top hat back to the Safe, because this contract shouldn't need to take ownership of the top hat in the first place.

Just use the fact that we add this contract as a module on the Safe, to proxy those calls to create new hats through the Safe.

@adamgall adamgall merged commit 3dfd2c6 into develop Oct 30, 2024
3 checks passed
@adamgall adamgall deleted the add-contract/DecentAutonomousAdminHat branch October 30, 2024 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants