Increase signal max uuid length

[ward-taoshi]

Increase signal uuid max length for multiple position/order closes.
allows close up to 2048 // 37 = 55 positions or orders

[github-actions]

🤖 Claude AI Code Review

Last reviewed on: 14:23:47

Summary

This PR increases the miner_order_uuid field max length from 256 to 2048 characters to support batch operations (closing up to 55 positions/orders). When UUIDs exceed standard length (36 chars), the code generates a new UUID for file system operations while preserving the original for processing.

✅ Strengths

• Clear purpose: Enables batch operations for multiple position/order closes
• Backwards compatible: Standard UUIDs (≤36 chars) continue to work as before
• Logging added for UUID mapping operations for debugging
• Minimal surface area change reduces risk

⚠️ Concerns

Critical:

1. UUID Mapping Loss (Lines 392-395)

   • The mapping between order_uuid and file_uuid is created but never persisted
   • If an error occurs, audit trail will be broken - no way to correlate the original order_uuid to the archived file
   • Example: Original UUID "pos1|pos2|pos3" maps to random file_uuid, but this mapping exists only in memory

2. Inconsistent Error Handling (Lines 466-475)

   • In the exception handler, file_uuid might not be defined if the error occurs before line 392
   • This could cause a NameError when trying to write to failed signals directory

3. Magic Number (Line 393)

   • Hard-coded 36 should be a named constant (e.g., STANDARD_UUID_LENGTH = 36)
   • The PR description mentions 2048 // 37 = 55 but uses 36 in code - inconsistency

Moderate:

4. No Validation on UUID Format (Line 388+)

   • Long UUIDs are accepted without format validation
   • Could lead to issues if malformed data is passed
   • Consider validating the structure if it's meant to be pipe-delimited UUIDs

5. Import Added but Standard UUID Used (Line 10)

   • import uuid added, but uuid.uuid4() generates a completely new UUID
   • Consider using a deterministic hash of the original order_uuid instead for reproducibility

💡 Suggestions

1. Persist UUID Mapping:

   file_uuid = order_uuid
   uuid_mapping = None
   if len(file_uuid) > STANDARD_UUID_LENGTH:
       file_uuid = str(uuid.uuid4())
       uuid_mapping = {"original": order_uuid, "file": file_uuid}
       bt.logging.info(f"Mapping long uuid to file uuid {file_uuid}")
       # Persist mapping to a lookup file/database
       self._persist_uuid_mapping(uuid_mapping)

2. Use Deterministic Hashing:

   import hashlib
   
   if len(file_uuid) > STANDARD_UUID_LENGTH:
       # Create deterministic UUID from hash
       hash_digest = hashlib.sha256(order_uuid.encode()).hexdigest()
       file_uuid = str(uuid.UUID(hash_digest[:32]))
       bt.logging.info(f"Hashed long uuid ({len(order_uuid)} chars) to file uuid {file_uuid}")

   This allows reproducible mapping without persistence.

3. Add Constant and Validation:

   STANDARD_UUID_LENGTH = 36
   MAX_UUID_LENGTH = 2048
   
   if not order_uuid or len(order_uuid) > MAX_UUID_LENGTH:
       raise ValueError(f"Invalid order_uuid length: {len(order_uuid)}")

4. Initialize file_uuid in Broader Scope:

   start_time = time.time()
   file_uuid = order_uuid  # Initialize here
   
   try:
       if len(file_uuid) > STANDARD_UUID_LENGTH:
           file_uuid = str(uuid.uuid4())
           # ... rest of logic

5. Add Documentation:

   • Update docstring for process_rest_order to explain the UUID mapping behavior
   • Document the expected format for concatenated UUIDs (appears to be pipe-delimited based on description)

🔒 Security Notes

1. Input Length Validation - While max_length is set to 2048 in the protocol, there's no explicit validation before processing. Consider adding bounds checking.

2. File Path Injection - fake_signal_file_path = f"/rest-api/{file_uuid}" could theoretically be vulnerable if file_uuid contains path traversal characters. The new random UUID generation actually improves security here, but validate if accepting the original long UUID.

3. Information Disclosure - Long UUIDs might contain sensitive concatenated data. Logging the full order_uuid could expose this. Consider truncating in logs:

   bt.logging.info(f"Mapping long uuid (length: {len(order_uuid)}) to file uuid {file_uuid}")

4. DOS Risk - 2048-char strings in database/protocol fields could be used for DOS. Ensure downstream systems can handle this length efficiently.

Additional Questions for PR Author:

1. What format are the concatenated UUIDs? (pipe-delimited, comma-separated?)
2. Is there a requirement to retrieve orders by original UUID later?
3. Have you tested the failure path where file_uuid might be undefined?
4. Why 2048 specifically? Is this an arbitrary choice or based on system constraints?

Recommendation: Request changes to address the UUID mapping persistence issue and error handling scope before merging.

[sli-tao]

should we add a dedicated field to the Signal model (e.g. position_ids: Optional[List[str]]) and keep miner_order_uuid as a unique identifier for the FLAT_ALL action itself. The protocol's miner_order_uuid max_length stays at 256

[sli-tao]

this might break the harvester