chore(vault): make pre-initialization access consistently return NotInitialized - #400
chore(vault): make pre-initialization access consistently return NotInitialized#400mac-dubem wants to merge 2 commits into
Conversation
|
@mac-dubem is attempting to deploy a commit to the Collins' projects Team on Vercel. A member of the Team first needs to authorize it. |
collinsezedike
left a comment
There was a problem hiding this comment.
Verified independently: checked this out, ran cargo clippy --all-targets -- -D warnings (clean) and cargo test -p meridian-vault --lib (32 passed, matches the PR's own claim). The ?-propagation is threaded correctly through every caller, and existing non-try_ test call sites still compile since Soroban's generated client wrapper auto-unwraps Result and panics on Err. Logic is correct.
Two things need fixing before merge.
PR Title check is failing. The title kept the bracketed issue-style format instead of conventional-commit format (type(scope): description). Since this repo squashes on merge, the PR title becomes the final commit message.
This branch predates #397 by about 33 minutes, which gitignored packages/contracts/**/test_snapshots/ and removed all tracked snapshot files. This PR still has 54 tracked snapshot files, including 10 new ones for the tests it adds. Please rebase onto current main and drop all of them from the diff, they're gitignored now and shouldn't be committed.
|
Hey @mac-dubem, checking in on this one since it's been a few days quiet. Are you still good to continue? If you're stuck on anything, let me know here and I can help. If you're no longer able to finish it, just say so and I'll pass the issue on to someone else. |
…nitialized Change get_admin, get_adapter, require_admin, set_paused, set_admin, set_adapter, deposit, withdraw, and get_total_assets to return ContractError::NotInitialized instead of panicking with .unwrap() when called before initialize(). Add 8 tests covering the uninitialized case for each affected function. Drop tracked test_snapshots (already gitignored after drydocs#397). Closes drydocs#375
ae61964 to
9a21977
Compare
collinsezedike
left a comment
There was a problem hiding this comment.
CI is failing on Commit Messages: the header is 79 characters, over the 72-char limit. Since this repo squash-merges using the PR title, both the commit header and the PR title need to be trimmed to 72 characters or under, e.g. chore(vault): return NotInitialized instead of panicking pre-init.
Summary
get_admin,get_adapter, andrequire_adminto returnContractError::NotInitializedinstead of panicking with.unwrap()when called beforeinitialize()set_paused,set_admin,set_adapter,deposit,withdraw, andget_total_assetsto propagate theNotInitializederrorThis makes error handling consistent — callers and off-chain indexers can match on
ContractError::NotInitializedinstead of special-casing which functions panic.Test plan
cargo clippy --all-targets -- -D warningspassescargo testpasses (32 vault tests, all passing)Closes #375