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

Fix base bridging and a race condition in key funder #3012

Merged
merged 3 commits into from
Dec 1, 2023

Conversation

tkporter
Copy link
Collaborator

@tkporter tkporter commented Dec 1, 2023

Description

Turns out bridging from L1 to Base has been broken the whole time :( it's because the version of @eth-optimism/sdk we were using was 1.8.0 (according to yarn.lock) which was released in late 2022 https://github.com/ethereum-optimism/optimism/releases/tag/%40eth-optimism%2Fsdk%401.8.0, far before Base was launched. So we'd always get this error in key funder because we rely on the optimism SDK knowing about Base's chain ID:

{"chain":"base","error":"Error: cannot get contract AddressManager for unknown L2 chain ID 8453, you must provide an address\n    at getOEContract (/hyperlane-monorepo/node_modules/@eth-optimism/sdk/src/utils/contracts.ts:58:11)\n    at getAllOEContracts (/hyperlane-monorepo/node_modules/@eth-optimism/sdk/src/utils/contracts.ts:121:46)\n    at new CrossChainMessenger (/hyperlane-monorepo/node_modules/@eth-optimism/sdk/src/cross-chain-messenger.ts:170:39)\n    at ContextFunder.bridgeToOptimism (/hyperlane-monorepo/typescript/infra/scripts/funding/fund-keys-from-deployer.ts:652:33)\n    at ContextFunder.bridgeToL2 (/hyperlane-monorepo/typescript/infra/scripts/funding/fund-keys-from-deployer.ts:637:23)\n    at processTicksAndRejections (node:internal/process/task_queues:96:5)\n    at async ContextFunder.bridgeIfL2 (/hyperlane-monorepo/typescript/infra/scripts/funding/fund-keys-from-deployer.ts:492:9)\n    at async gracefullyHandleError (/hyperlane-monorepo/typescript/infra/scripts/funding/fund-keys-from-deployer.ts:774:5)\n    at async /hyperlane-monorepo/typescript/infra/scripts/funding/fund-keys-from-deployer.ts:394:29\n    at async Promise.all (index 9)","level":"error","message":"Error bridging to L2"}

The fix was just to upgrade to a newer optimism SDK version

A mystery to me is that we'd get that log ^ but sometimes the key funder pod would show as having ran successfully. My best guess here is there was a race condition in fund when we had a variable local to the function called failureOccurred that many promises which were Promise.all'd would read and write to it via failureOccurred ||= await someFallibleFn(). I changed the logic here to not have multiple concurrent promises contend for the variable

Also made a small change to not include mantapacific in the list of relayer keys for the Hyperlane context. It's not ever used, and had a downstream effect of us trying to fund the Kathy key on mantapacific, which we don't want to actually do

Drive-by changes

n/a

Related issues

n/a

Backward compatibility

ye

Testing

ran locally

Copy link

changeset-bot bot commented Dec 1, 2023

⚠️ No Changeset found

Latest commit: 9b692ef

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@aroralanuk
Copy link
Contributor

Thanks for spotting the base funder issue!

@tkporter tkporter enabled auto-merge (squash) December 1, 2023 19:08
@tkporter tkporter merged commit 9fc0866 into main Dec 1, 2023
10 of 16 checks passed
@tkporter tkporter deleted the trevor/fix-base-bridging branch December 1, 2023 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants