Skip to content

Conversation

@jimmyc256
Copy link
Collaborator

Aptos Token V2 Processor 수정 사항 반영한 PR 입니다.
aptos-labs/aptos-indexer-processors-v2@f597601

@jimmyc256 jimmyc256 self-assigned this Jun 4, 2025
Copy link

@gemini-code-assist gemini-code-assist bot left a 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 CancelTokenOfferEvent and CancelOffer events 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 TokenV1Canceled type alias to track canceled token offers (line 25).
    • Added tokens_canceled parameter to RawCurrentTokenPendingClaim::from_write_set_change to include canceled offer data (line 148).
    • Modified from_write_set_change to check tokens_canceled for owner address information (line 174).
    • Updated panic message in from_write_set_change to include tokens_canceled data for better debugging (line 184).
  • rust/processor/src/db/common/models/token_v2_models/raw_v2_token_activities.rs
    • Imported TokenV1Canceled (line 13).
    • Added tokens_canceled parameter to RawTokenActivityV2::from_event to allow populating the map (line 206).
    • Modified CancelTokenOfferEvent handling to insert data into the tokens_canceled map (line 299).
    • Modified CancelOffer handling to insert data into the tokens_canceled map (line 330).
  • rust/processor/src/processors/token_v2_processor.rs
    • Imported TokenV1Canceled (line 14).
    • Initialized a mutable tokens_canceled AHashMap to collect canceled offer data (line 977).
    • Passed the tokens_canceled map to the RawTokenActivityV2::from_event call (line 1088).
    • Passed the tokens_canceled map to the RawCurrentTokenPendingClaim::from_write_set_change call (line 1258).
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

  1. 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.

Copy link

@gemini-code-assist gemini-code-assist bot left a 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_address in raw_token_claims.rs when falling back to tokens_claimed and tokens_canceled differs from the upstream commit. The current PR might incorrectly prioritize or overwrite addresses if a token ID appears in both maps, whereas upstream checks tokens_canceled only if tokens_claimed doesn't yield an address. (Severity: critical)
  • Code Duplication: In raw_v2_token_activities.rs, the handling of TokenEvent::CancelTokenOfferEvent and TokenEvent::CancelOffer is nearly identical, leading to code duplication. This could be refactored into a helper function. (Severity: medium)
  • Comment Typo: A comment in token_v2_processor.rs contains 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 the CancelTokenOfferEvent and CancelOffer blocks. 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.

Comment on lines +174 to +176
if let Some(token_canceled) = tokens_canceled.get(&token_data_id) {
maybe_owner_address = token_canceled.from_address.clone();
}

Choose a reason for hiding this comment

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

critical

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?

Suggested change
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();
}
}

Comment on lines +290 to 301
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
},

Choose a reason for hiding this comment

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

medium

The logic within this TokenEvent::CancelTokenOfferEvent match arm is very similar to the logic in the TokenEvent::CancelOffer arm (lines 321-332). Both involve:

  1. Cloning inner.token_id.token_data_id.
  2. Constructing a TokenActivityHelperV1.
  3. Inserting a clone of the helper into tokens_canceled.
  4. 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

Choose a reason for hiding this comment

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

medium

There seems to be a small typo or artifact (Add commentMore actions) at the end of this comment. Could you please remove it for clarity?

Suggested change
// Get cancel events for token v1 by table handleAdd commentMore actions
// Get cancel events for token v1 by table handle

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants