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 missing error to PriceFeedMock #981

Closed
wants to merge 3 commits into from

Conversation

0xAkuti
Copy link

@0xAkuti 0xAkuti commented Nov 7, 2024

Adds missing error definition for the error LZ_PriceFeed_NotAnOPStack which is used in PriceFeedMock._getL1LookupId() and currently causing issues (#978).

Error: 
Compiler run failed:
Error (7576): Undeclared identifier.
   --> lib/devtools/packages/test-devtools-evm-foundry/contracts/mocks/PriceFeedMock.sol:184:16:
    |
184 |         revert LZ_PriceFeed_NotAnOPStack(l2Eid);
    |                ^^^^^^^^^^^^^^^^^^^^^^^^^

The alternative would be to add it in ILayerZeroPriceFeed where the error LZ_PriceFeed_UnknownL2Eid which was previously used is defined, but since LZ_PriceFeed_NotAnOPStack seems to only be used in this mock and not in layerzero v2 adding it here seems to be the way to go.

@0xAkuti
Copy link
Author

0xAkuti commented Nov 18, 2024

Hi, any feedback on this? @janjakubnanista
Would be nice to merge it since we currently use a forked repo with the fix and would like to use the official one.

Copy link
Contributor

@janjakubnanista janjakubnanista left a comment

Choose a reason for hiding this comment

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

Hey @0xAkuti! Could you please include a changeset? You just need to run pnpm changeset from the root of the repo and mark this package as patch, thanks!

If you're at it I would also update the branch on top of the newest main from devtools, we included some bug fixes that remedied flaky test runs

@janjakubnanista janjakubnanista changed the title Add missing error to PriceFeedMock 🪲 Add missing error to PriceFeedMock Nov 18, 2024
@0xAkuti
Copy link
Author

0xAkuti commented Nov 19, 2024

Hi @janjakubnanista, I added the changeset and updated to the current main

@0xAkuti
Copy link
Author

0xAkuti commented Nov 19, 2024

It was added last week in the layerzero-v2 repo (LayerZero-Labs/LayerZero-v2#121) in ILayerZeroPriceFeed like I mentioned as the alternative approach in the top comment. So it's no longer needed here.

@0xAkuti
Copy link
Author

0xAkuti commented Nov 19, 2024

Resolved with LayerZero-Labs/LayerZero-v2#121

@0xAkuti 0xAkuti closed this Nov 19, 2024
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.

2 participants