feat: handling undeliverable contracts on seller side

[shev-titan]

This PR implements undeliverable contract detection and includes several improvements to contract management, error handling, and code quality.

🔧 Core Feature: Undeliverable Contract Detection

• Added undeliverable contract logic that detects when a contract cannot be fulfilled due to persistent destination connection issues
• Added IsUndeliverable() method to contract interface, exposed it in HTTP API
• FIxed Error() field in api to expose the latest delivery error

🐛 Bug Fixes

• Improved removeFullMiners() logic with proper input validation and edge case handling

Testing Considerations

• Verify undeliverable detection triggers after 2 consecutive cycles with destination errors
• Confirm contract stops gracefully when marked as undeliverable
• Test HTTP API returns correct undeliverable status (there is a (cycle duration = 5 min) delay for contract application status field)
• Test following purchase of the same contract
• Test changing destination from deliverable to undeliverable and vice-versa

[codereviewbot-ai]

The ApplicationStatus is derived from item.State().String(), which is noted as rw mutex cancelable. This implies potential read-write conflicts or race conditions if the state is accessed concurrently without proper synchronization. To ensure thread safety and data consistency, consider implementing locking mechanisms (such as mutexes) around accesses to the state, or ensure that the state is accessed in a thread-safe manner if not already handled within State() method.

[codereviewbot-ai]

The error handling in Error: errString(item.Error()) converts the error to a string, which is a common practice. However, it's important to ensure that this conversion does not omit necessary details about the error, which are crucial for effective debugging and logging. Consider enhancing the errString function to include more detailed error information or context, especially if item.Error() can contain multiple error states or complex error information.

[codereviewbot-ai]

The Error field is currently a string, which may not be the most effective way to handle errors, especially when the system needs to make decisions based on these errors. Consider using a more structured error handling approach, such as custom error types or error codes, which can be easily checked and handled programmatically.

Suggested Change:

type ContractError struct {
    Code    int
    Message string
}

Error ContractError

This change allows easier error handling and integration with automated systems, improving maintainability and reliability.

[codereviewbot-ai]

The fields Dest and PoolDest are used to store destination addresses and are both of type string. This could lead to potential data integrity issues if the values are not validated or sanitized properly. It's recommended to add validation checks to ensure these fields contain valid data before they are processed further in the system.

Suggested Change:
Implement validation functions that check the format and correctness of the data in Dest and PoolDest fields. This could prevent errors and security issues related to incorrect data handling.

[codereviewbot-ai]

Reinitializing channels (startCh, stopCh, doneCh) in the Reset method without ensuring that any previous goroutines using these channels have been properly terminated can lead to goroutine leaks and unpredictable behavior.

Recommendation: Ensure all goroutines are properly terminated before reinitializing the channels, or consider using a different approach to manage the lifecycle of these channels to prevent resource leaks.

[codereviewbot-ai]

The method StopFulfilling() is called within a conditional block, and the completion is awaited using <-c.ContractWatcherSellerV2.Done(). This introduces a potential blocking operation which could degrade performance if the Done() channel does not receive a signal promptly. Consider implementing a timeout mechanism or handling potential delays/errors more robustly to prevent the application from hanging.

Suggested Improvement:

select {
case <-c.ContractWatcherSellerV2.Done():
    // Proceed after successful stop
case <-time.After(timeoutDuration):
    // Handle timeout scenario
}

[codereviewbot-ai]

The error handling for StopFulfilling() is not addressed. If StopFulfilling() encounters an error or fails to execute as expected, the error is not captured or logged, which could lead to unmanaged states or issues within the application. It is crucial to handle such errors to maintain the integrity and reliability of the application.

Suggested Improvement:

if err := c.ContractWatcherSellerV2.StopFulfilling(); err != nil {
    c.log.Errorf("Failed to stop fulfilling: %s", err)
    return err
}

[codereviewbot-ai]

The methods BlockchainState(), ValidationStage(), and Error() expose internal state details directly, which could lead to security risks if not properly managed. Consider encapsulating these details more thoroughly and providing methods that offer decision-relevant information instead of direct state exposure. This approach can enhance security by limiting the accessibility of sensitive contract details and reducing the risk of misuse in broader application contexts.

[codereviewbot-ai]

The Error() method currently returns only the last error encountered, which might not be sufficient for robust error handling in complex systems. Consider implementing an error logging or aggregation mechanism that can provide a more comprehensive view of errors over the lifecycle of a contract. This enhancement will allow for better diagnosis and resolution of issues, contributing to more reliable contract management.