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

Interop #2585

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Interop #2585

wants to merge 4 commits into from

Conversation

isstuev
Copy link
Collaborator

@isstuev isstuev commented Feb 21, 2025

Description and Related Issue(s)

resolves #2588

Proposed Changes

added NEXT_PUBLIC_INTEROP_ENABLED env (optimistic only)

Checklist for PR author

  • I have tested these changes locally.
  • I added tests to cover any new functionality, following this guide
  • Whenever I fix a bug, I include a regression test to ensure that the bug does not reappear silently.
  • If I have added, changed, renamed, or removed an environment variable
    • I updated the list of environment variables in the documentation
    • I made the necessary changes to the validator script according to the guide
    • I added "ENVs" label to this pull request

@isstuev isstuev marked this pull request as ready for review February 24, 2025 17:29
@isstuev isstuev requested a review from tom2drum February 24, 2025 17:29
Copy link
Collaborator

Choose a reason for hiding this comment

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

The files names could be more descriptive; for example, use .env.optimism_interop_0.

@@ -461,6 +461,7 @@ This feature is **enabled by default** with the `coinzilla` ads provider. To swi
| NEXT_PUBLIC_ROLLUP_L2_WITHDRAWAL_URL | `string` | URL for L2 -> L1 withdrawals (Optimistic stack only) | Required for `optimistic` rollups | - | `https://app.optimism.io/bridge/withdraw` | v1.24.0+ |
| NEXT_PUBLIC_FAULT_PROOF_ENABLED | `boolean` | Set to `true` for chains with fault proof system enabled (Optimistic stack only) | - | - | `true` | v1.31.0+ |
| NEXT_PUBLIC_HAS_MUD_FRAMEWORK | `boolean` | Set to `true` for instances that use MUD framework (Optimistic stack only) | - | - | `true` | v1.33.0+ |
| NEXT_PUBLIC_INTEROP_ENABLED | `boolean` | Enables "Interop messages" page (Optimistic stack only) | - | `false` | `true` | v1.39.0+ |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this variable is specific to rollup, let's name it with the common prefix NEXT_PUBLIC_ROLLUP_*.

Comment on lines +1248 to +1255
// INTEROP
interop_messages: {
path: '/api/v2/optimism/interop/messages',
filterFields: [],
},
interop_messages_count: {
path: '/api/v2/optimism/interop/messages/count',
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is also better to prefix the names of these resources with optimistic_l2_, similar to optimistic_l2_deposits, optimistic_l2_withdrawals, etc. Additionally, place this code closer to the declaration of those resources.

Comment on lines +1467 to +1468
Q extends 'interop_messages' ? InteropMessageListResponse :
Q extends 'interop_messages_count' ? number :
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please move it to the ResourcePayloadRollups type

Comment on lines +3 to +5
// Modal,
// ModalContent,
// ModalCloseButton,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lets's remove it if it is unnecessary.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can likely eliminate the gaps between the arrow and address snippets in the final line of the item, making it resemble the token transfers on the transaction page.

image

const TxEntityInterop = ({ chain, hash, ...props }: Props) => {
const partsProps = distributeEntityProps(props);

const href = (chain?.instance_url && hash) ? chain.instance_url.replace(/\/$/, '') + route({
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the replace function is intended to remove the trailing slash from a URL, it will be more readable if we use the utility function lib/stripTrailingSlash.ts.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't the ad banner be covered with an overlay similar to other test cases?

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.

interop
2 participants