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

Add DepositMessageDelivered event #99

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

tsite
Copy link
Contributor

@tsite tsite commented Dec 4, 2023

This event allows bridges to efficiently query the deposit history for an address without requiring an external indexer.

@cla-bot cla-bot bot added the s label Dec 4, 2023
@yahgwai
Copy link
Contributor

yahgwai commented Dec 5, 2023

Just noting that this adds around 1.1k gas to all delayed messages and batch spending reports

@gzeoneth gzeoneth changed the base branch from main to develop December 5, 2023 11:16
@@ -203,6 +203,7 @@ abstract contract AbsBridge is Initializable, DelegateCallAware, IBridge {
baseFeeL1,
blockTimestamp
);
emit MessageDeliveredV2(kind, sender);
Copy link
Member

Choose a reason for hiding this comment

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

Might make more sense to have this event in the depositETH function so it does not add gas usage to other types of inbox messages. Can you elaborate more on the usecase of this feature?

Copy link
Contributor Author

@tsite tsite Dec 7, 2023

Choose a reason for hiding this comment

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

The main usecase is for a bridge application to show a user their deposits. Normally the MessageDelivered event is used for this, however since address is not indexed in this event, deposits for all users have to be queried which is much less efficient and may requires multiple requests. Agreed that the event only needs to be emitted for deposit events - updated the logic to reflect that, which should save on gas.

This event allows bridges to efficiently query the deposit history
for an address without requiring an external indexer.
@tsite tsite force-pushed the message_delivered_v2 branch from 39511bb to 187879e Compare December 7, 2023 10:19
only emit this event for deposit messages to save gas
@tsite tsite force-pushed the message_delivered_v2 branch from 187879e to a51c411 Compare December 7, 2023 10:24
@tsite tsite requested a review from gzeoneth December 7, 2023 10:25
@tsite tsite changed the title Add MessageDeliveredV2 event Add DepositMessageDelivered event Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants