-
Notifications
You must be signed in to change notification settings - Fork 0
feat: add support for canceled token offers in token processing #20
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
base: main
Are you sure you want to change the base?
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.
Hello @jimmyc256, 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!
Summary of Changes
Hello team, Gemini here with a summary of this pull request. The primary goal of this PR, as indicated by the title and description, is to enhance the Aptos Token V2 Processor to correctly handle and process events related to canceled token offers. This involves introducing new logic and data structures to track canceled offers alongside claimed ones, ensuring the processor maintains an accurate state for token activities, particularly concerning pending claims.
Highlights
- Support for Canceled Offers: Adds explicit support for processing
CancelTokenOfferEventandCancelOfferevents within the token V2 processor. - New Data Structure: Introduces a new type alias,
TokenV1Canceled, to store metadata for canceled token offers, similar to how claimed offers are tracked. - Integration into Processing Flow: Modifies the core token processing logic to collect and utilize information from canceled offer events when determining the state and ownership of tokens, especially in the context of pending claims.
Changelog
Click here to see the changelog
- rust/processor/src/db/common/models/token_v2_models/raw_token_claims.rs
- Added
TokenV1Canceledtype alias to track canceled token offers (line 25). - Added
tokens_canceledparameter toRawCurrentTokenPendingClaim::from_write_set_changeto include canceled offer data (line 148). - Modified
from_write_set_changeto checktokens_canceledfor owner address information (line 174). - Updated panic message in
from_write_set_changeto includetokens_canceleddata for better debugging (line 184).
- Added
- rust/processor/src/db/common/models/token_v2_models/raw_v2_token_activities.rs
- Imported
TokenV1Canceled(line 13). - Added
tokens_canceledparameter toRawTokenActivityV2::from_eventto allow populating the map (line 206). - Modified
CancelTokenOfferEventhandling to insert data into thetokens_canceledmap (line 299). - Modified
CancelOfferhandling to insert data into thetokens_canceledmap (line 330).
- Imported
- rust/processor/src/processors/token_v2_processor.rs
- Imported
TokenV1Canceled(line 14). - Initialized a mutable
tokens_canceledAHashMap to collect canceled offer data (line 977). - Passed the
tokens_canceledmap to theRawTokenActivityV2::from_eventcall (line 1088). - Passed the
tokens_canceledmap to theRawCurrentTokenPendingClaim::from_write_set_changecall (line 1258).
- Imported
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
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.
Offer made, then pulled away,
Processor sees the light of day.
Canceled state now known,
In data, clearly shown,
No pending claim can go astray.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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.
Code Review
This pull request aims to add support for canceled token offers in the token processing logic, aligning with changes from an upstream repository. The core idea is to track canceled V1 token offers and use this information, particularly when determining ownership for pending claims if a claim record was deleted due to cancellation.
Overall, the changes are in the right direction, but there's a critical logic discrepancy compared to the referenced upstream commit that needs to be addressed. Additionally, there are a couple of medium-severity issues related to code clarity and maintainability.
Summary of Findings
- Logic Discrepancy in Owner Resolution: The logic for determining
maybe_owner_addressinraw_token_claims.rswhen falling back totokens_claimedandtokens_canceleddiffers from the upstream commit. The current PR might incorrectly prioritize or overwrite addresses if a token ID appears in both maps, whereas upstream checkstokens_canceledonly iftokens_claimeddoesn't yield an address. (Severity: critical) - Code Duplication: In
raw_v2_token_activities.rs, the handling ofTokenEvent::CancelTokenOfferEventandTokenEvent::CancelOfferis nearly identical, leading to code duplication. This could be refactored into a helper function. (Severity: medium) - Comment Typo: A comment in
token_v2_processor.rscontains an artifact:Add commentMore actions. (Severity: medium) - Minor Inefficiency (Not Commented): In
raw_v2_token_activities.rs,inner.token_id.token_data_id.clone()is called twice consecutively in theCancelTokenOfferEventandCancelOfferblocks. This is a very minor inefficiency and was not commented on due to review settings. (Severity: low)
Merge Readiness
This pull request introduces valuable functionality for handling canceled token offers. However, there is a critical issue regarding the logic for resolving owner addresses in raw_token_claims.rs which deviates from the apparent source of truth in the upstream commit. This must be addressed to ensure correctness.
Additionally, there are a couple of medium-severity suggestions for improving code maintainability and clarity.
I recommend that the critical issue be resolved, and the medium-severity issues be considered, before this PR is merged. As an AI, I am not authorized to approve pull requests; please ensure further review and approval by authorized team members after addressing the feedback.
| if let Some(token_canceled) = tokens_canceled.get(&token_data_id) { | ||
| maybe_owner_address = token_canceled.from_address.clone(); | ||
| } |
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.
The current logic for determining maybe_owner_address when a table handle is not found in table_handle_to_owner differs from the upstream commit f5976019d227236739ca5d40935cfc503c9a78d6 that this PR aims to reflect.
Currently, if token_data_id exists in both tokens_claimed and tokens_canceled, the from_address from tokens_canceled will overwrite the one from tokens_claimed (if the else block for table_handle_to_owner is hit).
The upstream logic, however, only checks tokens_canceled if maybe_owner_address is still None after checking tokens_claimed. This prevents an unintentional overwrite and seems more aligned with the intent of using canceled offers as a fallback.
Could you update this logic to match the upstream behavior to ensure correctness, especially for cases where a token might (however unlikely) appear in both maps or to correctly prioritize claimed over canceled if both were somehow populated for the same token_data_id before this point?
| if let Some(token_canceled) = tokens_canceled.get(&token_data_id) { | |
| maybe_owner_address = token_canceled.from_address.clone(); | |
| } | |
| // if not in token claim events, try to get owner from token cancel events | |
| // (this is for the case where the offer was cancelled, and the claim record is deleted) | |
| if maybe_owner_address.is_none() { | |
| if let Some(token_canceled) = tokens_canceled.get(&token_data_id) { | |
| maybe_owner_address = token_canceled.from_address.clone(); | |
| } | |
| } |
| TokenEvent::CancelTokenOfferEvent(inner) => { | ||
| let token_data_id_struct = inner.token_id.token_data_id.clone(); | ||
| let helper = TokenActivityHelperV1 { | ||
| token_data_id_struct: inner.token_id.token_data_id.clone(), | ||
| property_version: inner.token_id.property_version.clone(), | ||
| from_address: Some(event_account_address.clone()), | ||
| to_address: Some(inner.get_to_address()), | ||
| token_amount: inner.amount.clone(), | ||
| }; | ||
| tokens_canceled.insert(token_data_id_struct.to_id(), helper.clone()); | ||
| helper | ||
| }, |
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.
The logic within this TokenEvent::CancelTokenOfferEvent match arm is very similar to the logic in the TokenEvent::CancelOffer arm (lines 321-332). Both involve:
- Cloning
inner.token_id.token_data_id. - Constructing a
TokenActivityHelperV1. - Inserting a clone of the helper into
tokens_canceled. - Returning the helper.
The main difference lies in how from_address is derived (Some(event_account_address.clone()) vs Some(inner.get_from_address())).
To improve maintainability and reduce duplication, would you consider refactoring this common logic into a private helper function within this impl RawTokenActivityV2 block? This function could take the differing parts (like from_address) as parameters.
For example, a helper like:
fn create_and_register_canceled_helper(
inner_token_id: &TokenIdType, // Or appropriate type for inner.token_id
inner_amount: &BigDecimal, // Or appropriate type for inner.amount
from_address_val: Option<String>,
to_address_val: Option<String>,
tokens_canceled: &mut TokenV1Canceled,
) -> TokenActivityHelperV1 {
let token_data_id_struct = inner_token_id.token_data_id.clone();
let helper = TokenActivityHelperV1 {
token_data_id_struct: token_data_id_struct.clone(),
property_version: inner_token_id.property_version.clone(),
from_address: from_address_val,
to_address: to_address_val,
token_amount: inner_amount.clone(),
};
tokens_canceled.insert(token_data_id_struct.to_id(), helper.clone());
helper
}This would make the match arms more concise.
| // Get claim events for token v1 by table handle | ||
| let mut tokens_claimed: TokenV1Claimed = AHashMap::new(); | ||
|
|
||
| // Get cancel events for token v1 by table handleAdd commentMore actions |
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.
Aptos Token V2 Processor 수정 사항 반영한 PR 입니다.
aptos-labs/aptos-indexer-processors-v2@f597601