Skip to content

fix: header event emits indexed and unindexed header #17

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

Merged
merged 6 commits into from
May 5, 2024

Conversation

prestwich
Copy link
Member

@prestwich prestwich commented May 2, 2024

indexed structs are serialized and hashed, and the original data is not included in the event. This adds a non-indexed Header struct param alongside, so that we keep access to the header data

@prestwich prestwich added the enhancement New feature or request label May 2, 2024
@prestwich prestwich requested a review from anna-carroll May 2, 2024 16:21
@prestwich prestwich self-assigned this May 2, 2024
@anna-carroll
Copy link
Contributor

i think maybe let's just do non-indexed?

@prestwich
Copy link
Member Author

having both allows us to still use it as a topic to find the specific event that admitted that header

@anna-carroll
Copy link
Contributor

anna-carroll commented May 2, 2024

yes but i'm wondering in what circumstances we would have the exact header information beforehand and want to use it to query the event. confirmBy field in the header in particular means that there isn't a situation where we'd be able to query for a header without knowing the specific information about that header beforehand.

i'm wondering if there's a way we could serve the need of "i know about a header, i want to find the event" while also servicing other more general & useful things.

i think it would be nice to have the following indexed:

  • uint256 rollupChainId;
  • uint256 sequence;

perhaps we break out the header info into individual event params and index those two?

@anna-carroll
Copy link
Contributor

anna-carroll commented May 2, 2024

i think it would be nice to have the following indexed:

  • uint256 rollupChainId;
  • uint256 sequence;

for example it would be nice to be able to say "give me all the blocks from a given chain" OR "give me all the 0th blocks that have been emitted, so i can see what chains there are"

but this would also serve the need of "i have a full header and i want to find the event" because (ruChainId, sequence) is a unique block identifier

@prestwich
Copy link
Member Author

so the new would be

event BlockSubmitted(
        address indexed sequencer, uint256 indexed rollupChainId, uint256 indexed sequence, BlockHeader header, bytes32 blockDataHash
    );

ya that sounds good to me

@anna-carroll
Copy link
Contributor

anna-carroll commented May 2, 2024

would we wanna do

event BlockSubmitted(
   address indexed sequencer, 
   uint256 indexed rollupChainId, 
   uint256 indexed sequence, 
   uint256 confirmBy, 
   uint256 gasLimit, 
   address rewardAddress, 
   bytes32 blockDataHash
);

just to avoid paying for re-emitting rollupChainId and sequence unnecessarily

@prestwich
Copy link
Member Author

this is fine. let's ship it

@anna-carroll anna-carroll merged commit 14bf33c into main May 5, 2024
@anna-carroll anna-carroll deleted the prestwich/header-in-event branch May 5, 2024 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants