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

Migrate Optimism Bridge Flows - Ready for review #4130

Merged
merged 11 commits into from
Aug 25, 2023

Conversation

henrystats
Copy link
Contributor

Thank you for contributing to Spellbook!

Please refer to the top of the readme in the root of Spellbook to learn how to contribute to Spellbook on DuneSQL.

@dune-eng
Copy link

Workflow run id 5918029859 approved.

@dune-eng
Copy link

Workflow run id 5918029936 approved.

@dune-eng
Copy link

Workflow run id 5919785506 approved.

@dune-eng
Copy link

Workflow run id 5919785490 approved.

@dune-eng
Copy link

Workflow run id 5919801973 approved.

@dune-eng
Copy link

Workflow run id 5919802016 approved.

@dune-eng
Copy link

Workflow run id 5919815364 approved.

@dune-eng
Copy link

Workflow run id 5919815312 approved.

@dune-eng
Copy link

Workflow run id 5919909370 approved.

@dune-eng
Copy link

Workflow run id 5919909338 approved.

@dune-eng
Copy link

Workflow run id 5919950534 approved.

@dune-eng
Copy link

Workflow run id 5919950576 approved.

@henrystats henrystats changed the title WIP: Migrate Optimism Bridge Flows Migrate Optimism Bridge Flows - Ready for review Aug 20, 2023
@dune-eng
Copy link

Workflow run id 5920329715 approved.

@dune-eng
Copy link

Workflow run id 5920329621 approved.

@henrystats
Copy link
Contributor Author

@chuxinh wanted to tag you to this as well incase you wanted to review... migrated the bridge flows spell and the spells that it relied on...

tested it on dune.com that it works correctly as well.

Copy link
Contributor

@chuxinh chuxinh left a comment

Choose a reason for hiding this comment

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

Some nits. Otherwise LGTM!

@@ -27,7 +29,7 @@ WITH bridge_events AS (
recipient_address,
trace_address, a.evt_index,
project_contract_address,
COALESCE(message_nonce_hash,'unknown') AS transfer_id,
message_nonce_hash AS transfer_id, -- removing the coalesce since this isn;t a unique key
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
message_nonce_hash AS transfer_id, -- removing the coalesce since this isn;t a unique key
message_nonce_hash AS transfer_id, -- removing the coalesce since this isn't a unique key

- &block_month
name: block_month
description: Month of the block time
Copy link
Contributor

Choose a reason for hiding this comment

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

add extra line

) a ("name","chain","nativeCurrency","chainId","networkId","infoURL","explorer")
Copy link
Contributor

Choose a reason for hiding this comment

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

add extra line

- &block_month
name: block_month
description: Month of the block time
Copy link
Contributor

Choose a reason for hiding this comment

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

add extra line

DENSE_RANK() OVER (PARTITION BY evt_tx_hash ORDER BY evt_index ASC) AS msg_index

FROM {{ source ('ovm_optimism', 'L2CrossDomainMessenger_evt_RelayedMessage') }}
{% if is_incremental() %}
WHERE evt_block_time >= (NOW() - interval '14 days')
WHERE evt_block_time >= (NOW() - interval '14' Day)
Copy link
Contributor

Choose a reason for hiding this comment

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

add a line below

@dune-eng
Copy link

Workflow run id 5967873954 approved.

@dune-eng
Copy link

Workflow run id 5967874167 approved.

@dune-eng
Copy link

Workflow run id 5967877005 approved.

@dune-eng
Copy link

Workflow run id 5967876904 approved.

@henrystats
Copy link
Contributor Author

changes made @chuxinh

@henrystats
Copy link
Contributor Author

@jeff-dude the Wallet summary spell relies on this getting merged first so I could add bridged funds to it...

This bridge flows spell is ready for review / merging.

Copy link
Member

@jeff-dude jeff-dude left a comment

Choose a reason for hiding this comment

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

one minor comment below

models/chain_info/chain_info_chain_ids.sql Outdated Show resolved Hide resolved
@jeff-dude jeff-dude self-assigned this Aug 25, 2023
@jeff-dude jeff-dude added in review Assignee is currently reviewing the PR DuneSQL migration labels Aug 25, 2023
@dune-eng
Copy link

Workflow run id 5978038627 approved.

@dune-eng
Copy link

Workflow run id 5978038566 approved.

@dune-eng
Copy link

Workflow run id 5978449540 approved.

@dune-eng
Copy link

Workflow run id 5978449679 approved.

Copy link
Member

@jeff-dude jeff-dude left a comment

Choose a reason for hiding this comment

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

thank you!

@jeff-dude jeff-dude merged commit d0f747b into duneanalytics:main Aug 25, 2023
3 checks passed
@jeff-dude jeff-dude removed the in review Assignee is currently reviewing the PR label Aug 25, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Aug 25, 2023
@henrystats henrystats deleted the migrate-bridge-flows branch September 4, 2023 15:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants