fix(contracts): use typed errors in deposit and execute_rebalance ins…

[Rampop01]

Closes #6

Description

Replaced the untyped panic!() calls in deposit() and execute_rebalance() with structured error handling (Result<(), Error>). This allows on-chain callers and SDK clients to catch and handle errors gracefully.

Changes Made

• contracts/src/types.rs: Added InvalidAmount (variant 9) to the Error enum.
• contracts/src/lib.rs:
  • Updated the signature of deposit() and execute_rebalance() to return Result<(), Error>.
  • Replaced panic!("Amount must be positive") with return Err(Error::InvalidAmount).
  • Replaced panic!("Emergency stop active") with return Err(Error::EmergencyStop).
  • Replaced panic!("Cooldown active") with return Err(Error::CooldownActive).
  • Replaced panic!("Stale price data") with return Err(Error::StaleData).
• contracts/src/test.rs: Updated all relevant tests to use the client's try_deposit and try_execute_rebalance methods. They now assert typed errors (e.g., assert_eq!(result, Err(Ok(Error::InvalidAmount)))) instead of using #[should_panic].

[Uchechukwu-Ekezie]

Hi @Rampop01 Thanks for the contribution. Moving from panic!() to typed errors is definitely an improvement for contract usability and testability.

A few things I'd like clarified before approval:

Since deposit() and execute_rebalance() now return Result<(), Error>, have all downstream callers and generated client bindings been updated accordingly?
The issue scope appears relatively small, but this PR touches 13 files with a fairly large diff. Could you highlight any non-error-handling changes that were introduced?
Can you confirm all panic paths within these functions were converted to typed errors and that no panic-based validation remains?

Also, I noticed the checks are currently failing. Could you share the cause of the CI failure and whether it's related to this change?

[Rampop01]

Hi @Rampop01 Thanks for the contribution. Moving from panic!() to typed errors is definitely an improvement for contract usability and testability.

A few things I'd like clarified before approval:

Since deposit() and execute_rebalance() now return Result<(), Error>, have all downstream callers and generated client bindings been updated accordingly? The issue scope appears relatively small, but this PR touches 13 files with a fairly large diff. Could you highlight any non-error-handling changes that were introduced? Can you confirm all panic paths within these functions were converted to typed errors and that no panic-based validation remains?

Also, I noticed the checks are currently failing. Could you share the cause of the CI failure and whether it's related to this change?

Thanks for the review! Here are the clarifications regarding the points you raised:

1. Downstream Callers & Client Bindings

The repository's internal backend and frontend applications do not currently invoke deposit() or execute_rebalance() directly using auto-generated client bindings, so no internal code was broken. However, external SDK clients or developers relying on the generated bindings will need to regenerate their TypeScript clients (e.g., using soroban contract bindings typescript) to accommodate the new Result<(), Error> return type.

2. Large Diff (13 Files)

There are no non-error-handling logical changes. The large diff footprint is because Soroban automatically updates interface definition snapshots (JSON files in contracts/test_snapshots/) when cargo test is run following an ABI change. The return type change from () to Result<(), Error> forced 10 of these snapshot files to regenerate. The only source files manually modified were lib.rs, types.rs, and test.rs.

3. Panic Paths

Good catch! I double-checked the logic and found one remaining panic (panic!("Missing price data");) inside execute_rebalance(). I've just pushed an additional commit to convert this to a new typed error (Error::MissingPriceData). All panic-based validation paths inside these functions have now been thoroughly converted to typed errors.

4. CI Failure

The CI failure was caused by pre-existing, unrelated TypeScript compilation errors in backend/src/api/routes.ts (due to missing imports and uninitialized service variables) which caused npm run build to fail. This had nothing to do with the smart contract changes. I went ahead and fixed these backend TypeScript errors and pushed the fix to this branch, so the CI should now pass successfully!

[Rampop01]

@Uchechukwu-Ekezie CI pass now, kindly review and merge

[Uchechukwu-Ekezie]

Thanks for your breakdown am a good job done @Rampop01

I will be adding more issue soon please I will be glad if you can still apply