Reject invalid inputs in price utilities

[cjonas9]

What

In xdr/prices.go::Price.Cheaper(), prices/main.go::MulFractionRoundDown() and prices/main.go::mulFractionRoundUp(), Int32 and Int64 values are cast to uint64 values, which would wrap to large, meaningless values. Then, there are several big.Rat() calls where the denominator is checked to not be zero, which would cause an unrecoverable panic.

For xdr/prices.go, because fixing these would cause breaking changes, this adds equivalent Try*() methods that implement their corresponding methods with errors returned instead of panicking/returning invalid output. The new methods are unit tested as the old ones are, along with a new test to ensure invalid output is rejected.

The old methods are marked as deprecated, except for Price.String() as this implements the Stringer interface -- this case is still also changed to be handled without panic.

Why

This adds defensive programming methods for developers utilizing these functions, but note that functions are already called safely in Stellar services.

Known limitations

N/A

[Copilot]

Pull request overview

This PR hardens price utilities against invalid inputs that could previously lead to uint wraparound (from negative signed ints) or panics (e.g., big.NewRat with a zero denominator), adding explicit validation and error paths.

Changes:

• Add xdr.Price.Validate() and guard String, Equal, Cheaper, Normalize, and Invert against invalid prices.
• Reject negative inputs in price.MulFractionRoundDown and price.mulFractionRoundUp via a new ErrNoNegatives error.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
xdr/price.go	Adds price validation and uses it to prevent panics/wraparound in Price helpers.
price/main.go	Adds a negative-input error and enforces non-negative operands in fraction multiplication helpers.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[tamirms]

Below are specific comments with actionable suggestions and evidence from the Go standard library.

---

1. Remove fmt.Printf (Libraries should not print to stdout)

The Issue:
In xdr/price.go, the String() method prints to stdout:

fmt.Printf("cannot stringify invalid price: %v\n", err)

Why this matters:
Libraries should never write directly to standard output. It spams the logs of consuming applications, interferes with CLI tools that rely on stdout for data piping, and bypasses the application's structured logging system.

Standard Library Evidence:
Look at net/http. When the HTTP server encounters an error, it doesn't print to stdout; it uses a configurable *log.Logger (see Server.ErrorLog). If that isn't set, it degrades gracefully or returns the error to the caller. It never assumes it owns os.Stdout.

Actionable Suggestion:
Remove the fmt.Printf call entirely. The String() method should simply return the descriptive string. If the application cares about the invalid state, they can log it themselves using the returned value.

---

2. Make String() Safe, Not Silent

The Issue:
Currently, String() returns "INVALID" when validation fails. This hides the actual data causing the problem, making debugging harder for the user.

Standard Library Evidence:
Look at net.IP.String(). If an IP address is nil or empty, it doesn't panic, nor does it return a generic "INVALID" string. It returns "<nil>".
Similarly, reflect.Value.String() returns <invalid Value> for zero values. The convention is to use angle brackets <> to denote a meta-status that isn't a valid value.

Actionable Suggestion:
Return a string that describes the invalid state including the raw values. This satisfies fmt.Stringer without masking the data.

func (p Price) String() string {
    if err := p.Validate(); err != nil {
        // Returns something like "<invalid price (10/0): denominator cannot be zero>"
        return fmt.Sprintf("<invalid price (%d/%d): %v>", p.N, p.D, err)
    }
    // ... existing logic
}

---

3. Avoid Silent Failures in Boolean Methods (Cheaper, Equal)

The Issue:
The PR modifies Cheaper and Equal to return false (and print to stdout) if the price is invalid.

if err := p.Validate(); err != nil {
    fmt.Printf(...)
    return false
}

Why this is risky:
This creates a silent failure. In a financial context, if Price A is invalid, reporting that it is not cheaper than Price B (returning false) implies that Price B is cheaper or equal. This is mathematically false (the comparison is undefined) and could trick a trading algorithm into executing a bad trade.

Standard Library Evidence:
When the encoding/json package encounters a situation where it cannot guarantee correctness (like a syntax error), it doesn't try to "guess" or return a default zero-value; it returns a SyntaxError.
While we cannot change the signature of Cheaper to return an error without breaking the API (v1 compatibility), we should arguably keep the panic or explicitly document undefined behavior rather than returning a misleading boolean.

Actionable Suggestion:
Since breaking the API is likely not an option, the safest "Band-Aid" is:

1. Deprecate the unsafe methods: Mark Cheaper and Equal as deprecated.
2. Add Safe Alternatives: Introduce SafeCheaper(p Price) (bool, error).
3. Fail loudly in the legacy method: If you must keep Cheaper without errors, do not print to stdout. Just return false but ensure the deprecation notice warns users that invalid prices yield undefined results.

---

4. Endorsing the ErrNoNegatives Pattern

The Good:
The changes in price/main.go (e.g., MulFractionRoundDown) where you added checks for ErrNoNegatives are perfect.

Standard Library Evidence:
This mirrors strconv.Atoi. It doesn't panic on bad input; it returns 0 and a *NumError. By returning ErrNoNegatives, you are adhering to the "Errors are Values" philosophy of Go.

Actionable Suggestion:
This part of the PR is solid. Just ensure there is a unit test case specifically for this negative input to ensure the error is returned as expected.