chore(contracts): replace panic! with typed errors - #435
Conversation
|
@Caneryy is attempting to deploy a commit to the Collins' projects Team on Vercel. A member of the Team first needs to authorize it. |
|
@Caneryy Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
collinsezedike
left a comment
There was a problem hiding this comment.
CI is failing on two things:
Commit Messages: header is 73 characters, over the 72-char limit. Needs shortening, e.g.chore(contracts): replace panic! with typed errors.Soroban Contract Tests:cargo fmt --all -- --checkfails on the import block inblend-adapter/src/lib.rs. Runcargo fmtand push the result.
The PR title (feat(meridian): chore-replace-panic-with-typed-errors-in-router-) also needs fixing to match, since this repo squash-merges using the PR title as the commit message.
The implementation itself is correct, I ran the full test suite and clippy locally and both pass clean. Just these two fixed before merge.
|
This PR's body doesn't follow CONTRIBUTING.md's PR body format (see "Pull Request Process" > "PR body format"). The Summary section should describe what changed and why, not restate the issue title. The Test plan checkboxes should be checked once you've actually run Please update the PR body to reflect what you actually did and verified before this is merged. |
0ea1ad1 to
4e926e2
Compare
4e926e2 to
1a4f260
Compare
collinsezedike
left a comment
There was a problem hiding this comment.
Summary of what was fixed to get this ready:
- Rebased onto current
main(this branch was from before several recent merges landed) and reapplied just the actual contracts changes cleanly. - Removed two now-duplicate/obsolete tests:
blend-adapteranddefindex-adaptereach still had their old#[should_panic]reinitialization test from before this PR's own conversion, which either collided with or contradicted the new typed-error test this PR adds for the same thing. - Commit header and PR title shortened to fit the 72-character limit (first attempt was 73 characters, 1 over).
- PR body: filled in Summary and Test plan properly.
The core conversion itself (panic! to typed ContractError/RouterError across router and both adapters) was correct from the start.
Thank you for the contribution. Feel free to grab another open issue.
Merging now.
Summary
router::rebalance()and both adapters'initialize()used rawpanic!strings, inconsistent with the vault's typedContractErrorpattern ([Contracts] Replace panic strings with a structured ContractError enum #338), and harder for off-chain callers/indexers to handleRouterError, adapter-specificContractError::AlreadyInitialized) and converted all three to returnResultinstead of panicking#[should_panic]Test plan
cargo testpasses across all four contract crates (vault, blend-adapter, defindex-adapter, router)cargo clippy --all-targets -- -D warningspassescargo fmt --all -- --checkpassesCloses #433