Skip to content

fix: P-1485 update all fields to optional in ...ResponseData of pumpx… #3427

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

Merged
merged 7 commits into from
Apr 29, 2025

Conversation

wli-pro
Copy link
Contributor

@wli-pro wli-pro commented Apr 24, 2025

… api to avoid decode error

Context

Labels

Please apply following PR-related labels when appropriate:

  • C0-breaking: if your change could break the existing client, e.g. API change, critical logic change
  • C1-noteworthy: if your change is non-breaking, but is still worth noticing for the client, e.g. reference code improvement

How (Optional)

Testing Evidences

Please attach any relevant evidences if applicable

Copy link

linear bot commented Apr 24, 2025

@wli-pro wli-pro requested review from Copilot and BillyWooo April 24, 2025 05:59
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates several Pumpx-related API endpoints to treat response fields as optional in order to avoid decode errors, and it refactors error handling by introducing a new helper function for option unwrapping. Key changes include:

  • Replacing manual Option checks with the new check_and_get_option_response_data function across multiple modules.
  • Updating Pumpx API types to use Option types for previously required fields.
  • Improving error handling and logging in native task handling and cross-chain executors.

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tee-worker/omni-executor/rpc-server/src/methods/pumpx/transfer_widthdraw.rs Updated option handling for response data and user_id checks
tee-worker/omni-executor/rpc-server/src/methods/pumpx/submit_swap_order.rs Refactored gas type and trade info validations using the new helper
tee-worker/omni-executor/rpc-server/src/methods/pumpx/export_wallet.rs Refactored response data extraction with check_and_get_option_response_data
tee-worker/omni-executor/rpc-server/src/methods/pumpx/common.rs Added the helper function for option response data checking
tee-worker/omni-executor/rpc-server/src/error_code.rs Added a new error code constant for user trade info retrieval failures
tee-worker/omni-executor/pumpx/src/pumpx_api/types.rs Made several response fields optional to align with new API behavior
tee-worker/omni-executor/native-task-handler/src/lib.rs Improved error handling and extracted verify_google_code logic
tee-worker/omni-executor/intent/executors/cross-chain/src/lib.rs Updated gas info extraction using the new Rust pattern syntax
Comments suppressed due to low confidence (2)

tee-worker/omni-executor/rpc-server/src/methods/pumpx/transfer_widthdraw.rs:1

  • [nitpick] Consider renaming the file to 'transfer_withdraw.rs' if 'widthdraw' is an unintended typo to improve clarity and consistency.
File name: transfer_widthdraw.rs

tee-worker/omni-executor/native-task-handler/src/lib.rs:737

  • [nitpick] Ensure that the error message format for google code verification failures is consistent across modules; consider standardizing the phrasing for easier troubleshooting.
send_error("Failed to verify google code within NativeTask::PumpxExportWallet".to_string(), response_sender, NativeTaskError::PumpxApiError(PumpxApiError::GoogleCodeVerificationFailed));

@BillyWooo
Copy link
Collaborator

Please remove the Option for the data field:

pub struct ApiResponse<T: Codec> {
	pub code: u32,
	pub message: String,
	pub data: Option<T>,
}

Besides, there is another new set of omni rpc calls. Basically they are copied over from pumpx. Please double check and also adapt the error code there.

@wli-pro wli-pro force-pushed the update-all-fields-optional branch from bec60fc to 2448a75 Compare April 28, 2025 10:06
Copy link
Collaborator

@BillyWooo BillyWooo left a comment

Choose a reason for hiding this comment

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

thanks.

@BillyWooo BillyWooo enabled auto-merge (squash) April 29, 2025 06:05
@BillyWooo BillyWooo merged commit b7b5a5a into dev Apr 29, 2025
21 checks passed
@BillyWooo BillyWooo deleted the update-all-fields-optional branch April 29, 2025 08:24
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