-
Notifications
You must be signed in to change notification settings - Fork 1
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
Extend Dune Sync appdata on Gnosis Chain #99
Conversation
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.
Changes look correct but I am not confident at all with that part of the code base.
Is there some way to test changes locally? If not, lets test in our cluster.
else: | ||
if chain == "gnosis": |
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.
is this equivalent to an elif
?
if chain == "mainnet": | ||
return int( | ||
# KeyError here means the query has been modified and column no longer exists | ||
# IndexError means no results were returned from query (which is unlikely). | ||
(await self.fetch(QUERIES["LATEST_APP_HASH_BLOCK"].query))[0][ | ||
"latest_block" | ||
] | ||
) | ||
return int( | ||
# KeyError here means the query has been modified and column no longer exists | ||
# IndexError means no results were returned from query (which is unlikely). | ||
(await self.fetch(QUERIES["LATEST_APP_HASH_BLOCK"].query))[0][ | ||
(await self.fetch(QUERIES["LATEST_APP_HASH_BLOCK_GNOSIS"].query))[0][ | ||
"latest_block" | ||
] | ||
) |
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.
using an if .. elif ...
seems clearer here, throwing an exception is the chain name is not correct.
(the pythonic way might be to use a match ... case ... case ...
block)
genesis_bl = 15310317 | ||
if chain == "mainnet": | ||
genesis_bl = 12153262 |
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.
using an if .. elif ...
seems clearer here, as above
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.
All of the if conditions wouldn't be necessary if you used a single, parameterized, query and just pass "chain" as the network parameter:
See here:
-- https://dune.com/queries/3865197 | ||
select | ||
max(call_block_number) as latest_block | ||
from | ||
gnosis_protocol_v2_gnosis.GPv2Settlement_call_settle |
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.
You really should not need another query for this:
See here: https://dune.com/queries/3870399
-- App Hashes: https://dune.com/queries/3865206 | ||
-- MIN(first_block_seen) = 15310317 | ||
with | ||
app_hashes as ( | ||
select | ||
min(call_block_number) first_seen_block, | ||
get_json_object(trade, '$.appData') as app_hash | ||
from gnosis_protocol_v2_gnosis.GPv2Settlement_call_settle | ||
lateral view explode(trades) as trade | ||
group by app_hash | ||
) | ||
select | ||
app_hash, | ||
first_seen_block | ||
from app_hashes | ||
where first_seen_block > '{{BlockFrom}}' | ||
and first_seen_block <= '{{BlockTo}}' |
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.
Same comment as above.
Superseded by #103 |
This PR will attempt to replicate the mainnet setup in order to sync appdata from Gnosis Chain on Dune. Although wasteful, we need this data soon and as we are not very comfortable/familiar with Dune sync, this seems like the fastest way out.
This is marked as draft as it is incomplete.