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

feat: add support L2 block range lookup for Orbit chains #317

Merged
merged 33 commits into from
Aug 17, 2023

Conversation

brtkx
Copy link
Contributor

@brtkx brtkx commented Aug 10, 2023

No description provided.

@cla-bot cla-bot bot added the cla-signed label Aug 10, 2023
@brtkx brtkx changed the title chore: getBlockRangesForL1Block chore: support Orbit block range lookup Aug 10, 2023
@brtkx brtkx requested review from spsjvc and gzeoneth August 10, 2023 16:26
@brtkx brtkx changed the title chore: support Orbit block range lookup chore: support L2 block range lookup for Orbit chains Aug 11, 2023

while (start <= end) {
// Calculate the midpoint of the current range.
const mid = start + Math.floor((end - start) / 2)
Copy link
Member

Choose a reason for hiding this comment

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

I am a little bit unsure about these casting between BigNumber and number, it should be fine and I used it somewhere else in the sdk too.

src/lib/utils/lib.ts Outdated Show resolved Hide resolved
src/lib/message/L2ToL1MessageNitro.ts Outdated Show resolved Hide resolved
src/lib/utils/lib.ts Outdated Show resolved Hide resolved
src/lib/utils/lib.ts Outdated Show resolved Hide resolved
src/lib/utils/lib.ts Outdated Show resolved Hide resolved
src/lib/utils/lib.ts Outdated Show resolved Hide resolved
src/lib/utils/lib.ts Outdated Show resolved Hide resolved
@gzeoneth gzeoneth self-requested a review August 16, 2023 12:28
Copy link
Member

@gzeoneth gzeoneth left a comment

Choose a reason for hiding this comment

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

mostly lgtm, some comments

maxL2Block?: number | 'latest'
}): Promise<number | undefined> {
if (!isArbitrumChain(provider)) {
throw new Error('Arbitrum provider is required.')
Copy link
Member

Choose a reason for hiding this comment

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

I suggest we assume the provider is L1 if its not Arbitrum, and return forL1Block

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can do

Comment on lines +60 to +67
export const isArbitrumChain = async (provider: Provider): Promise<boolean> => {
try {
await ArbSys__factory.connect(ARB_SYS_ADDRESS, provider).arbOSVersion()
} catch (error) {
return false
}
return true
}
Copy link
Member

Choose a reason for hiding this comment

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

might want to do some caching here or in the network object, can do later

src/lib/utils/lib.ts Outdated Show resolved Hide resolved
tests/unit/utils.test.ts Outdated Show resolved Hide resolved
@spsjvc spsjvc changed the title chore: support L2 block range lookup for Orbit chains feat: add support L2 block range lookup for Orbit chains Aug 16, 2023
@brtkx brtkx marked this pull request as ready for review August 16, 2023 16:32
Copy link
Member

@gzeoneth gzeoneth left a comment

Choose a reason for hiding this comment

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

LGTM

@spsjvc spsjvc merged commit a273089 into main Aug 17, 2023
@spsjvc spsjvc deleted the l2-l1-message-orbit-support branch August 17, 2023 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants