Skip to content

Conversation

@adamegyed
Copy link
Contributor

Motivation

For retrieving previously set owners by validation ID, it would help to index the new signer. Additionally, since there's only one event type emitted by the SingleSignerValidation contract, we can mark the event as anonymous and index the previous signer, rather than the event type.

Solution

Update the SignerTransferred event to be anonymous, and index the previous and new signers.

Update tests to check using the new event signature instead, specifically coming from the SingleSignerValidation module address.

@adamegyed adamegyed requested a review from a team August 14, 2024 17:33
@adamegyed adamegyed force-pushed the adam/update-ssv-event branch from ceae64b to 1413909 Compare August 14, 2024 18:00
address indexed account, uint32 indexed entityId, address previousSigner, address newSigner
);
address indexed account, uint32 indexed entityId, address indexed newSigner, address indexed previousSigner
) anonymous;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to index signers?
They would cost probably at least 700 more gas for those two.
In the real world use case, how many times would a signer be transfered?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it'd be less than 700 since we make the event anonymous. But since we already index the validation ID (acccount, entityid) why do we need to index the signers to find previously set owners @adamegyed ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this goes from 3 topics -> 4, so an increase of 375 gas. To answer the question of why we're indexing this, it's to be able to do a reverse lookup from the signer address to which validation id it was installed with. Given that use case though, it's not super valuable to index to previous signer - what do y'all think of making the previousSigner not indexed, but keeping the newSigner indexed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

So the improvement in devX is from get all account SignerTransfer events and filter out new signer address => get the event where the new signer is 0x0000...
That is a lot more convenient.
Sounds good on keeping newSigner and rid previousSigner index.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I updated the event to have the following signature:

event SignerTransferred(address indexed account, uint32 indexed entityId, address indexed newSigner, address previousSigner) anonymous;

@adamegyed adamegyed force-pushed the adam/update-ssv-event branch from 1413909 to 6c9e8b8 Compare August 15, 2024 14:48
@adamegyed adamegyed force-pushed the adam/update-ssv-event branch from 6c9e8b8 to 5d4cfbf Compare August 15, 2024 21:54
@adamegyed adamegyed merged commit 94baff3 into develop Aug 15, 2024
@adamegyed adamegyed deleted the adam/update-ssv-event branch August 15, 2024 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants