DAT-64/aptos-stablecoin #78
Conversation
Summary of ChangesHello @gregoriustanleyy, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request establishes a robust data infrastructure for Aptos stablecoins. It introduces new dbt models to curate stablecoin metadata and track their mint and burn activities, providing a comprehensive view of stablecoin supply dynamics on the Aptos blockchain. The changes involve adding seed data, defining silver-level metadata and event tables, and a gold-level dimension table, all designed for incremental updates and efficient querying. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces models for Aptos stablecoins, including metadata and mint/burn events. The overall logic is sound, but I have a few suggestions for improvement.
In silver__stablecoins_metadata.sql, I found a critical issue where the deduplication logic is only applied during incremental runs, which could lead to primary key violations on a full refresh. I've also suggested populating a timestamp column that is currently NULL.
In silver__stablecoins_mint_burn.sql, I've proposed a refactoring to reduce code duplication and improve maintainability.
Please see the detailed comments for suggestions.
| {% if is_incremental() %} | ||
| WHERE modified_timestamp >= ( | ||
| SELECT MAX(modified_timestamp) | ||
| FROM {{ this }} | ||
| ) | ||
| QUALIFY ROW_NUMBER() OVER ( | ||
| PARTITION BY token_address | ||
| ORDER BY COALESCE(crosschain_modified_timestamp, metadata_modified_timestamp) DESC | ||
| ) = 1 | ||
| {% endif %} |
There was a problem hiding this comment.
The QUALIFY clause is inside an {% if is_incremental() %} block, which means it only runs for incremental loads. On a full refresh, if a token_address exists in both crosschain_stablecoins and seed_stablecoins, this model will produce duplicate records for that token_address, causing a violation of the unique_key constraint. The QUALIFY clause should be applied unconditionally to ensure deduplication.
Additionally, the WHERE clause within the incremental block is redundant because incremental filtering is already handled within the crosschain_stablecoins and seed_stablecoins CTEs and can be removed for clarity.
QUALIFY ROW_NUMBER() OVER (
PARTITION BY token_address
ORDER BY COALESCE(crosschain_modified_timestamp, metadata_modified_timestamp) DESC
) = 1| c.peg_type, | ||
| c.peg_mechanism, | ||
| COALESCE(pr.is_verified, FALSE) AS is_verified, | ||
| NULL AS is_verified_modified_timestamp, |
There was a problem hiding this comment.
The column is_verified_modified_timestamp is hardcoded to NULL. The downstream model defi__dim_stablecoins consumes this column. To provide more meaningful data, this should be populated with the timestamp when the verification status was last updated.
You can achieve this by modifying the join on price__ez_prices_hourly (lines 85-91) to also select the modified_timestamp and then use it here.
For example, you can change the subquery to:
...
LEFT JOIN (
SELECT
token_address,
is_verified,
modified_timestamp AS is_verified_modified_timestamp,
ROW_NUMBER() OVER (PARTITION BY token_address ORDER BY hour DESC) AS rn
FROM {{ ref('price__ez_prices_hourly') }}
) pr ON c.token_address = pr.token_address AND pr.rn = 1
...Then you can use pr.is_verified_modified_timestamp here.
pr.is_verified_modified_timestamp,| ) | ||
|
|
||
| SELECT | ||
| block_number, | ||
| block_timestamp, | ||
| tx_hash, | ||
| version, | ||
| success, | ||
| event_index, | ||
| event_type, | ||
| event_resource, | ||
| metadata_address, | ||
| symbol, | ||
| name, | ||
| decimals, | ||
| sender_address, | ||
| receiver_address, | ||
| owner_address, | ||
| amount_unadj, | ||
| amount, | ||
| {{ dbt_utils.generate_surrogate_key(['tx_hash', 'event_index']) }} AS stablecoins_mint_burn_id, | ||
| SYSDATE() AS inserted_timestamp, | ||
| SYSDATE() AS modified_timestamp, | ||
| '{{ invocation_id }}' AS _invocation_id, | ||
| source_modified_timestamp | ||
| FROM mint_events | ||
|
|
||
| UNION ALL | ||
|
|
||
| SELECT | ||
| block_number, | ||
| block_timestamp, | ||
| tx_hash, | ||
| version, | ||
| success, | ||
| event_index, | ||
| event_type, | ||
| event_resource, | ||
| metadata_address, | ||
| symbol, | ||
| name, | ||
| decimals, | ||
| sender_address, | ||
| receiver_address, | ||
| owner_address, | ||
| amount_unadj, | ||
| amount, | ||
| {{ dbt_utils.generate_surrogate_key(['tx_hash', 'event_index']) }} AS stablecoins_mint_burn_id, | ||
| SYSDATE() AS inserted_timestamp, | ||
| SYSDATE() AS modified_timestamp, | ||
| '{{ invocation_id }}' AS _invocation_id, | ||
| source_modified_timestamp | ||
| FROM burn_events |
There was a problem hiding this comment.
The final two SELECT statements are identical, differing only in the CTE they select from (mint_events and burn_events). This code duplication can be avoided by first using UNION ALL on the two event CTEs into a new final CTE and then selecting from that result. This improves maintainability, as any future changes to the final column list will only need to be made in one place.
),
final_events AS (
SELECT * FROM mint_events
UNION ALL
SELECT * FROM burn_events
)
SELECT
block_number,
block_timestamp,
tx_hash,
version,
success,
event_index,
event_type,
event_resource,
metadata_address,
symbol,
name,
decimals,
sender_address,
receiver_address,
owner_address,
amount_unadj,
amount,
{{ dbt_utils.generate_surrogate_key(['tx_hash', 'event_index']) }} AS stablecoins_mint_burn_id,
SYSDATE() AS inserted_timestamp,
SYSDATE() AS modified_timestamp,
'{{ invocation_id }}' AS _invocation_id,
source_modified_timestamp
FROM final_events
| GROUP BY tx_hash, metadata_address | ||
| ), | ||
|
|
||
| mint_txs AS ( |
There was a problem hiding this comment.
I dont think this makes sense. Your definition of "mint" looks like it's really "transaction with more deposits than withdraws". Does this always hold true for the inbound bridge transactions?
There was a problem hiding this comment.
this model will be used purely for native aptos mints/burns. the mint/burn or locked stablecoins in bridge platforms will be on a different model silver__stablecoin_supply_contracts
No description provided.