Repository navigation
Conversation
Move admin.require_auth() ahead of name/symbol/decimals checks while keeping AlreadyInit first, add mock_auths front-runner coverage, and update README/testnet docs so they no longer claim initialize is unauthenticated. Closes YieldAnchor#8
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
A newly added regression test uses #[should_panic] and should assert the specific auth-failure result to avoid false positives.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
What changed in this PR
This PR tightens the YieldVault::initialize authorization semantics by ensuring the selected admin’s signature is required before metadata validation, and adds regression tests + documentation to pin the intended “option 1” guarantee from issue #8.
Changes:
- Move
admin.require_auth()to run immediately after theAlreadyInitcheck and before metadata validation. - Add initialization regression tests covering foreign-admin front-run attempts and auth-before-metadata behavior.
- Update deployment/testnet documentation and the contract README to reflect the guarantee and residual self-install window.
| File | Description |
|---|---|
| docs/deployment/testnet.md | Updates Testnet deployment caveats to reflect the admin self-authorization guarantee and residual self-install window. |
| contracts/yield_vault/src/lib.rs | Reorders require_auth ahead of metadata validation and adds regression tests for front-running/auth ordering. |
| contracts/yield_vault/README.md | Documents the initialization authorization guarantee and its limits. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| #[test] | ||
| #[should_panic] | ||
| fn initialize_panics_without_admin_auth() { | ||
| let env = Env::default(); | ||
| let admin = Address::generate(&env); | ||
| let asset = env | ||
| .register_stellar_asset_contract_v2(admin.clone()) | ||
| .address(); | ||
| let vault = env.register_contract(None, YieldVault); | ||
| let client = YieldVaultClient::new(&env, &vault); | ||
|
|
||
| // No mock_all_auths: require_auth must abort. | ||
| client.initialize( | ||
| &admin, | ||
| &asset, | ||
| &String::from_str(&env, "YieldAnchor Vault"), | ||
| &String::from_str(&env, "yVAULT"), | ||
| &6, | ||
| ); | ||
| } |
|
Reviewed the diff: |
…auth Close the self-install window by requiring deployer+salt to re-derive this contract address (BadDeployer otherwise) and deployer authorization, while keeping admin.require_auth from option 1. Thread salt through deploy helpers, clients, and both deploy scripts; update docs and tests accordingly.
ayush8620
changed the title
Closes #8
Summary
Implements both guarantees from issue #8 on
initialize:AlreadyInitand deployer binding, requireadmin.require_auth()(when admin ≠ deployer) before metadata validation.deployer+ creationsalt, re-derive the contract address, reject mismatches withBadDeployer, and require deployer authorization.Together these close the self-install capture window: a non-deployer cannot initialize even when self-signing as admin. No Soroban SDK major bump and no
__constructor.Why this differs from #24
AlreadyInit→ address bind → auth → metadataAlreadyInit→ address bind → metadata → authmock_auths, auth-before-BadName, missing-authErr(Err(_)))Changes
contracts/yield_vault/src/lib.rs— new ABIinitialize(deployer, salt, admin, …),BadDeployer, both auths, regression testscontracts/yield_vault/README.md/docs/deployment/testnet.md— document both guaranteespackages/{constants,shared-types,validation,contract-clients}— error code, schemas, salt helpers, client/deploy threadingscripts/deploy/deploy-yield-vault.{ts,sh}— pass salt through create + initializeTests
BountyHub
PR targets the open BountyHub bounty on #8 from
ayush8620.