Skip to content

Conversation

BillyWooo
Copy link
Collaborator

Extend omni_getHyperliquidSignatureData with support for the SendAsset action type, enabling generalized token transfers across perp DEXs, spot balances, users, and subaccounts (testnet-only feature).

Changes:

  • Add SendAsset variant to HyperliquidActionType enum with fields for destination, source_dex, destination_dex, token, amount, and from_sub_account
  • Add SendAsset variant to HyperliquidAction response enum
  • Implement SendAsset action handler with address validation for destination and optional from_sub_account
  • Import SendAsset struct from hyperliquid-rust-sdk
  • Update SDK dependency to BillyWooo/hyperliquid-rust-sdk branch usdClassTransfer_sendAsset
  • Add comprehensive tests: signature generation, JSON deserialization, and sub-account handling

The implementation follows the same patterns as existing actions (ApproveAgent, Withdraw3, ApproveBuilderFee), ensuring consistency with EIP-712 signature generation and error handling.

This is not yet merged on official hyperliquid-rust-sdk. So use fork repo branch.

Extend omni_getHyperliquidSignatureData with support for the SendAsset action type, enabling generalized token transfers across perp DEXs, spot balances, users, and subaccounts (testnet-only feature).

Changes:
- Add SendAsset variant to HyperliquidActionType enum with fields for destination, source_dex, destination_dex, token, amount, and from_sub_account
- Add SendAsset variant to HyperliquidAction response enum
- Implement SendAsset action handler with address validation for destination and optional from_sub_account
- Import SendAsset struct from hyperliquid-rust-sdk
- Update SDK dependency to BillyWooo/hyperliquid-rust-sdk branch usdClassTransfer_sendAsset
- Add comprehensive tests: signature generation, JSON deserialization, and sub-account handling

The implementation follows the same patterns as existing actions (ApproveAgent, Withdraw3, ApproveBuilderFee), ensuring consistency with EIP-712 signature generation and error handling.
@BillyWooo BillyWooo self-assigned this Oct 16, 2025
@linear
Copy link

linear bot commented Oct 16, 2025

@claude
Copy link

claude bot commented Oct 16, 2025

Claude finished @BillyWooo's task —— View job


Code Review for SendAsset Implementation

Comprehensive analysis of the SendAsset action type implementation for HyperliquidSignatureData.

Todo List:

  • Read repository guidelines (CLAUDE.md)
  • Analyze changed files for code quality
  • Review for potential bugs and issues
  • Check security concerns
  • Evaluate performance considerations
  • Assess test coverage
  • Provide comprehensive feedback

📋 Overall Assessment

The implementation is well-structured and follows established patterns in the codebase. The code quality is high with proper error handling, validation, and comprehensive testing.

Strengths

  1. Consistent Pattern Following: The SendAsset implementation mirrors existing actions (ApproveAgent, Withdraw3, ApproveBuilderFee) ensuring consistency
  2. Comprehensive Validation: Proper Ethereum address validation for destination and conditional validation for from_sub_account
  3. Excellent Test Coverage: Includes signature generation, JSON deserialization, and sub-account handling tests
  4. Proper Error Handling: Uses the established DetailedError pattern with descriptive messages
  5. Security Considerations: Address validation prevents malformed inputs

🔍 Detailed Review

Code Quality: Excellent

  • Clean separation of concerns with enum variants
  • Consistent naming conventions
  • Proper use of serde attributes for JSON handling
  • Well-documented test cases

Security Analysis: Good

  • ✅ Ethereum address validation for destination (get_hyperliquid_signature_data.rs:328-329)
  • ✅ Conditional validation for from_sub_account when not empty (get_hyperliquid_signature_data.rs:331-334)
  • ✅ Uses existing auth verification patterns
  • ✅ EIP-712 signature generation maintains security standards

Performance Considerations: Optimal

  • Minimal overhead added to existing flow
  • Validation only occurs when needed (from_sub_account only validated if not empty)
  • Efficient reuse of existing generate_eip712_signature function

Test Coverage: Comprehensive

  • ✅ Signature generation test (test_send_asset_action_signature)
  • ✅ JSON deserialization tests with and without sub-account
  • ✅ Domain, struct hash, and EIP-712 signing hash validation
  • ✅ Testnet chain ID usage (998) consistent with feature description

🔧 Minor Suggestions

  1. Documentation Enhancement: Consider adding inline comments explaining the purpose of empty string defaults for source_dex, destination_dex, and from_sub_account

  2. Validation Consistency: In get_hyperliquid_signature_data.rs:294 and get_hyperliquid_signature_data.rs:328, consider using consistent variable naming for validation results (both currently use let _ =)

🚨 Issues Found: None Critical

No critical bugs, security vulnerabilities, or performance issues identified. The implementation is production-ready.

📝 Specific Code Comments

File: get_hyperliquid_signature_data.rs:47-54

SendAsset {
    destination: String,
    source_dex: String,
    destination_dex: String,
    token: String,
    amount: String,
    from_sub_account: String,
},

✅ Good: All fields properly typed as String for JSON compatibility

File: get_hyperliquid_signature_data.rs:331-334

if !from_sub_account.is_empty() {
    let _ = validate_ethereum_address(&from_sub_account, "from_sub_account")
        .map_err(|e| e.to_error_object())?;
}

✅ Excellent: Smart conditional validation - only validates when sub-account is specified

File: get_hyperliquid_signature_data.rs:546-572
The test uses testnet chain ID (998) which aligns with the PR description that this is a "testnet-only feature"
✅ Good: Proper test setup for intended environment

🎯 Final Recommendation

APPROVE - This is a high-quality implementation that:

  • Follows established patterns
  • Includes proper validation and error handling
  • Has comprehensive test coverage
  • Introduces no security vulnerabilities
  • Maintains performance standards

The code is ready for production deployment on testnets as intended.


let _ = validate_ethereum_address(&destination, "destination")
.map_err(|e| e.to_error_object())?;
// Validate from_sub_account if it's not empty
if !from_sub_account.is_empty() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This if seems redundant, validate_ethereum_address would fail anyways

@BillyWooo BillyWooo merged commit b3e5e60 into dev Oct 20, 2025
20 checks passed
@BillyWooo BillyWooo deleted the p-1729-add-one-more-hyperliquidaction-type-sendasset branch October 20, 2025 10:32
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