Update Order Attributes

[sli-tao]

Taoshi Pull Request

Description

• Add value and quantity attributes to order

Related Issues (JIRA)

[Reference any related issues or tasks that this pull request addresses or closes.]

Checklist

• I have tested my changes on testnet.
• I have updated any necessary documentation.
• I have added unit tests for my changes (if applicable).
• If there are breaking changes for validators, I have (or will) notify the community in Discord of the release.

Reviewer Instructions

[Provide any specific instructions or areas you would like the reviewer to focus on.]

Definition of Done

• Code has been reviewed.
• All checks and tests pass.
• Documentation is up to date.
• Approved by at least one reviewer.

Checklist (for the reviewer)

• Code follows project conventions.
• Code is well-documented.
• Changes are necessary and align with the project's goals.
• No breaking changes introduced.

Optional: Deploy Notes

[Any instructions or notes related to deployment, if applicable.]

/cc @mention_reviewer

[github-actions]

🤖 Claude AI Code Review

Last reviewed on: 14:32:45

Summary

This PR adds value and quantity attributes to orders, refactoring the system from leverage-only orders to support three types of order specifications: leverage, USD value, or base asset quantity. This is a significant architectural change affecting order processing, position management, and slippage calculations across the entire codebase.

✅ Strengths

• Comprehensive test coverage: Test files properly updated with account_size and DEFAULT_ACCOUNT_SIZE constants
• Backward compatibility: Maintains support for legacy leverage-based orders while adding new functionality
• Systematic refactoring: Changes are applied consistently across the codebase
• Good separation of concerns: New parse_order_quantity() static method centralizes conversion logic
• Test setup improvements: Proper cleanup of test directories before creating PositionManager instances

⚠️ Concerns

CRITICAL - Incomplete Code

Line: vali_objects/position.py:57

unre

The position.py file appears to be truncated mid-line. This will cause immediate syntax errors and break the build.

CRITICAL - Missing Validation in Signal Handler

Lines: mining/run_receive_signals_server.py:61-63

leverage=float(data["leverage"]) if data.get("leverage") is not None else None,
value=float(data["value"]) if data.get("value") is not None else None,
quantity=float(data["quantity"]) if data.get("quantity") is not None else None,

No validation that exactly ONE of these fields is set. The validation only exists in validator.py:parse_order_quantity(), but miners could send invalid combinations directly to the signal receiver.

CRITICAL - Breaking Change Without Migration

The removal of capital parameter from multiple constructors (BacktestManager, PriceSlippageModel) could break existing code:

• Line: neurons/backtest_manager.py:106 - Removed capital parameter
• Line: neurons/backtest_manager.py:188 - PriceSlippageModel no longer accepts capital

No deprecation warnings or migration guide provided.

HIGH - Potential Division by Zero

Lines: neurons/validator.py:1037-1042

if existing_position.account_size <= 0:
    bt.logging.warning(f"Invalid account_size...")
    existing_position.account_size = ValiConfig.MIN_CAPITAL

This fallback is good, but the warning suggests this shouldn't happen. The root cause should be investigated - why would positions have invalid account sizes?

HIGH - Slippage Calculation Order Changed

Lines: neurons/validator.py:1063-1066

order.usd_base_rate = usd_base_price
order.quote_usd_rate = self.live_price_fetcher.get_quote_usd_conversion(order, existing_position.position_type)
net_portfolio_leverage = self.position_manager.calculate_net_portfolio_leverage(miner_hotkey)
order.slippage = PriceSlippageModel.calculate_slippage(order.bid, order.ask, order)

Previously slippage was calculated before adding the order to position. The order matters for timing and could affect slippage calculations. Needs verification.

MEDIUM - Inconsistent Error Handling

Line: neurons/validator.py:1096

if sum(fields_set) != 1:
    raise ValueError("Exactly one of 'leverage', 'value', or 'quantity' must be set")

Uses generic ValueError instead of domain-specific exceptions like SignalException used elsewhere in the file.

MEDIUM - Parameter Removed Without Default

Line: neurons/validator.py:874

OrderSource.MAX_ORDERS_PER_POSITION_CLOSE)  # account_size parameter removed

The account_size parameter was removed from this call but the function signature still exists. Could cause issues if callers expect this parameter.

MEDIUM - Magic Number in Test

Line: tests/vali_tests/test_position_splitting.py:282

self.assertEqual(result[0].orders[1].leverage, -2.0)

Changed from -3.0 to -2.0 without explanation. This might be fixing a bug, but the change is unexplained and could hide issues.

LOW - Removed TODO Without Resolution

Lines: runnable/generate_request_minerstatistics.py:709-710

# TODO [remove on 2025-10-02] 70 day grace period --> reset upper bound to bucket_end_time_ms
# Removed along with grace period logic

The TODO suggested this would be removed on 2025-10-02, but it's being removed now. Is this intentional?

💡 Suggestions

Fix the Truncated File

# vali_objects/position.py:57
unrealized_pnl: float = 0.0  # in USD (complete this line)

Add Validation to Signal Receiver

# mining/run_receive_signals_server.py:58
leverage = data.get("leverage")
value = data.get("value")
quantity = data.get("quantity")

fields_set = sum([x is not None for x in (leverage, value, quantity)])
if fields_set != 1:
    raise Exception("Exactly one of 'leverage', 'value', or 'quantity' must be set")

signal = Signal(
    trade_pair=TradePair.from_trade_pair_id(signal_trade_pair_str),
    leverage=float(leverage) if leverage is not None else None,
    value=float(value) if value is not None else None,
    quantity=float(quantity) if quantity is not None else None,
    order_type=OrderType.from_string(data["order_type"].upper())
)

Add Type Hints

# neurons/validator.py:1085
@staticmethod
def parse_order_quantity(
    signal: Dict[str, Any], 
    usd_base_conversion: float, 
    trade_pair: TradePair, 
    portfolio_value: float
) -> float:

Document Breaking Changes

Create a migration guide in the PR description or a separate MIGRATION.md file documenting:

• Removal of capital parameter
• New order quantity parsing logic
• Changes to slippage calculation timing

Add Bounds Checking

# neurons/validator.py:1104
quantity = (value * usd_base_conversion) / trade_pair.lot_size
if quantity <= 0:
    raise ValueError(f"Invalid quantity calculated: {quantity}")
return quantity

Improve Test Clarity

# tests/vali_tests/test_position_splitting.py:282
# Add comment explaining why leverage changed from -3.0 to -2.0
# e.g., "Expected leverage is -2.0 after position netting logic correction"
self.assertEqual(result[0].orders[1].leverage, -2.0)

🔒 Security Notes

1. Input Validation: The new fields (value, quantity) accept float inputs without bounds checking. Consider adding:

   • Maximum value limits to prevent overflow
   • Minimum non-zero values
   • Sanity checks against account size

2. REST API Changes: New /collateral/query-withdraw endpoint added with validation, but ensure rate limiting is applied to prevent abuse.

3. Float Precision: Financial calculations using floats throughout. Consider using decimal.Decimal for monetary values to avoid rounding errors.

4. Signature Validation: Good that withdrawal endpoint validates signatures, but ensure the nonce mechanism prevents replay attacks.

📋 Additional Recommendations

1. Version Bump: Subnet version bumped from 7.0.0 to 7.0.1, but changes seem more significant (minor vs patch). Consider if this should be 7.1.0.

2. Database Migration: If positions are persisted, ensure existing positions can be loaded with new net_value and net_quantity fields defaulting appropriately.

3. Integration Testing: This PR would benefit from end-to-end integration tests covering the three order types (leverage, value, quantity).

4. Performance: Multiple calls to live_price_fetcher.get_usd_base_conversion() - consider caching if performance becomes an issue.

5. Documentation: Update API documentation for miners explaining the three order specification methods and their use cases.

🎯 Verdict

BLOCK - Critical issues must be fixed before merge:

• Complete the truncated position.py file
• Add validation to signal receiver
• Verify slippage calculation timing change doesn't break behavior
• Document breaking changes

Once these are addressed, this is a well-structured refactoring that modernizes the order system.