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

Emit Event on Shared Signer Configuration #456

Merged
merged 1 commit into from
Jul 10, 2024

Conversation

nlordell
Copy link
Collaborator

@nlordell nlordell commented Jul 4, 2024

Fixes hats-finance#3

This PR adds an event that is emitted on shared signer configuration. Note that the event is emitted in the context of the configured Safe account. In order to prevent event topic0 collisions, we chose an explicitly verbose name.

With respect to indexing, we explicitly chose not to index any of the fields so that it matches the Created event:

/**
* @notice Emitted when a new signer is created.
* @param signer The signer address.
* @param x The x-coordinate of the public key.
* @param y The y-coordinate of the public key.
* @param verifiers The P-256 verifiers to use.
*/
event Created(address indexed signer, uint256 x, uint256 y, P256.Verifiers verifiers);

To re-iterate on the reasons we chose to not index some of the fields (same reasoning applies to Created event above):

  • x and y: These are single-use public key coordinates attached to a passkey. It doesn't make sense to filter events by "all events with x value", or similarly "all events with y value". Furthermore, you can compute the signer (which is indexed in the Created event) from the key coordinates, so you can to some degree search for signers for a given key coordinate.
  • verifiers: This felt more like a configuration than something that Dapps would need to filter by. Why it might be interesting to see on chain which verifiers are being used, tools like Dune can help with this, and the additional cost of an indexed field is not worth it in our opinion.

This PR adds an event that is emitted on shared signer configuration.
Note that the event is emitted in the context of the configured Safe
account. In order to prevent event `topic0` collisions, we chose an
explicitely verbose name.
@nlordell nlordell requested a review from a team as a code owner July 4, 2024 15:34
@nlordell nlordell requested review from akshay-ap, mmv08 and remedcu and removed request for a team July 4, 2024 15:34
@nlordell nlordell requested a review from mmv08 July 5, 2024 11:03
@nlordell nlordell merged commit 37def80 into main Jul 10, 2024
9 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jul 10, 2024
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Does not emit event after writing into storage
3 participants