-
Notifications
You must be signed in to change notification settings - Fork 1.2k
refactor(host-contracts): rename ACL delegation methods and events #1079
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
base: main
Are you sure you want to change the base?
Conversation
vm.prank(sender); | ||
vm.expectEmit(address(acl)); | ||
emit ACLEvents.DelegatedAccount( | ||
emit ACLEvents.DelegatedForUserDecryption( |
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.
is it possible to change the test name of the test function?
delegate, | ||
contractAddress, | ||
delegationCounter, | ||
++delegation.delegationCounter, |
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.
is it possible to double check that the counter is indeed incremented inside storage, using vm.load cheatcode. Same thing in revoke test.
https://getfoundry.sh/reference/cheatcodes/load/
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.
Forget about using vm.load cheatcode, since this is inside nested mapping, too much headache.
Instead, just extend the ACL contract as a MockACL one which will expose easily a getter view function giving access at least to the struct counter, or even better the whole struct itself to check all its fields are correctly being updated as expected. For this I advise you to reuse same pattern as the foundry test for HCULimit, using an HCULimitMock contract extending original one here https://github.com/zama-ai/fhevm/blob/main/host-contracts/test/hcuLimit/HCULimit.t.sol#L17
So in summary we need 2 things:
- we need previous test with the
MockACL
checking that theDelegation
struct in storage is being updated as expected after each delegate/revoke tx (within a chain of delegate/revoke). - as discussed before lunch, also test that within a similar (or same) chain of delegate/revoke calls in some custom order, each event emits an increasing counter incrementing by
1
each time.
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.
2 small requests
} | ||
AclContractEvents::Initialized(_) | ||
| AclContractEvents::DelegatedAccount(_) | ||
| AclContractEvents::DelegatedForUserDecryption(_) |
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.
Can't we keep the same name as function ?
delegateForUserDecryption
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 it is preferable to keep same convention as already existing events, such as Initialized
, Allowed
etc.
Refs https://github.com/zama-ai/fhevm-internal/issues/474