-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
DUX-798 ronin: initiate katana dex #7280
Conversation
This reverts commit db5ba99.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fyi @Hosuke
i made a decision to not use the uni v2 macro here, as many column names have _
prefix and i didn't want to modify the macro for lots of if blockchain = 'ronin' then
_ +
type of logic. it would also then run all models which use the macro both in CI and prod.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we'd still want to use the macro we can perform any needed column renames in a CTE and then pass the cte references in the macro.
with evt_swap as (
select
_amount1Out as amount1Out
,_amount1In as amount1In
,...
)
, evt_paircreated as (
select
...
)
{{
uniswap_compatible_v2_trades(
blockchain = 'ronin',
project = 'katana',
version = '1',
Pair_evt_Swap = 'evt_swap`,
Factory_evt_PairCreated = 'evt_paircreated'
)
}}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both approaches are ok for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i actually started down this approach via CTE renames, but then also realized the source tables we pass in have {{ }}
wrapped around them in the macro itself. i don't think passing a CTE into there will work with those in place.
my suggestion is we get this integrated, then consider a cleanup on the macro to prepare for these edge cases.
CI is likely to fail on node crash. successful run can be found in commit logs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.✅
No description provided.