Skip to content

archiver stores L1ToL2 pending messages#642

Merged
Maddiaa0 merged 3 commits into
masterfrom
rk/archiverl1Tol2
May 23, 2023
Merged

archiver stores L1ToL2 pending messages#642
Maddiaa0 merged 3 commits into
masterfrom
rk/archiverl1Tol2

Conversation

@rahul-kothari

Copy link
Copy Markdown
Contributor

Description

Next step is the sequencer actually consuming the said messages - this will be a separate PR since this is big enough already.

Fixes #520

Checklist:

  • I have reviewed my diff in github, line by line.
  • Every change is related to the PR description.
  • I have linked this pull request to the issue(s) that it resolves.
  • There are no unexpected formatting changes, superfluous debug logs, or commented-out code.
  • The branch has been merged or rebased against the head of its merge target.
  • I'm happy for the PR to be merged at the reviewer's next convenience.

@rahul-kothari rahul-kothari force-pushed the rk/archiverl1Tol2 branch 2 times, most recently from cf159bd to bd10daa Compare May 22, 2023 15:20
public getPendingL1ToL2Messages(take: number): Promise<L1ToL2Message[]> {
// todo: @rahul https://github.com/AztecProtocol/aztec-packages/issues/529 - change this so that sequencer actually actually consumes messages sorted by fee or another value
// upon consumption, the messages are removed from the store
return Promise.resolve(this.pendingL1ToL2Messages.slice(0, take));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does this decrease the size of the existing pendingMessages array? Or does it leave the sampled data there.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

sampled for now - will remove in #529

Comment thread yarn-project/archiver/src/archiver/eth_log_handlers.ts
Comment thread yarn-project/types/src/l1_to_l2_message.ts
Comment thread yarn-project/archiver/src/archiver/archiver.test.ts Outdated
Comment thread yarn-project/archiver/src/archiver/archiver.ts
Comment thread yarn-project/archiver/src/archiver/archiver.test.ts Outdated
Comment thread yarn-project/types/src/l1_to_l2_message.ts
Comment thread yarn-project/archiver/src/archiver/data_retrieval.ts Outdated
blockUntilSynced: boolean,
currentBlockNumber: bigint,
searchStartBlock: bigint,
) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Add a type for the return value.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah I tried but it gets quite ugly (which is why none of the methods here have a return type.
Basically you add a return type like : Promise<{ nextEthBlockNumber: bigint; retrievedData: L1ToL2Message[]}> and then the linter says Missing JDoc statement - except the linter wants the jdoc statement in the return statement itself (L143) as opposed to on L135.

@Maddiaa0 Maddiaa0 May 23, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You could make a generic type for each:

/**
 * Data retreived from logs
 */
type RollupLog<T> = {
  /**
   * The next block number.
   */
  nextEthBlockNumber: bigint;
  /**
   * The data returned.
   */
  retrievedData: T[];
}

Then have the return type be:

func something(): Promise<RollupLog<L1ToL2Message>> {}

should do the trick and appease the linter

Comment thread yarn-project/archiver/src/archiver/eth_log_handlers.ts Outdated
Comment thread yarn-project/archiver/src/archiver/eth_log_handlers.ts Outdated

@spalladino spalladino left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nothing to add from my end on top of @Maddiaa0 and @LHerskind comments. Feel free to loop in @spypsy if you want a review from an Otter more familiar with the archiver than myself!

Comment thread yarn-project/archiver/src/archiver/archiver.test.ts Outdated
Comment thread yarn-project/archiver/src/archiver/archiver.test.ts Outdated
blockUntilSynced: boolean,
currentBlockNumber: bigint,
searchStartBlock: bigint,
) {

@Maddiaa0 Maddiaa0 May 23, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You could make a generic type for each:

/**
 * Data retreived from logs
 */
type RollupLog<T> = {
  /**
   * The next block number.
   */
  nextEthBlockNumber: bigint;
  /**
   * The data returned.
   */
  retrievedData: T[];
}

Then have the return type be:

func something(): Promise<RollupLog<L1ToL2Message>> {}

should do the trick and appease the linter

Comment thread yarn-project/archiver/src/archiver/eth_log_handlers.ts Outdated
@rahul-kothari rahul-kothari requested a review from Maddiaa0 May 23, 2023 14:51
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.

extend archiver to pull L1 -> L2 events from message box

5 participants