-
Notifications
You must be signed in to change notification settings - Fork 3
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
Conversation
Add contract
| DecentAutonomousAdminHat
- break out create method into seperate function
contracts/DecentHats.sol
Outdated
uint256 hatId = _createHat( | ||
hatsProtocol, | ||
adminHatId, | ||
hat, | ||
topHatAccount, | ||
eligibilityAddress | ||
); | ||
|
||
IHatsElectionEligibility(eligibilityAddress).elect( | ||
hat.termedParams[0].termEndDateTs, | ||
hat.termedParams[0].nominatedWearers | ||
); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 elect
ion doesn't depend on the new hatId
, is what i'm saying.
contracts/DecentHats.sol
Outdated
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 | ||
); | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
…ct, and do some light refactoring
…ry" to "erc6551Registry"
Adds DecentAutonomousAdmin Contract
Adds DecentHats_0_2_0 Contract
Closes #103
Closes #104
Closes #105