feat(pool): add public getters for admin, invoice_contract, escrow_contract - #676
Conversation
…ntract Add get_admin(), get_invoice_contract(), and get_escrow_contract() public read-only accessors, matching the symmetry already present for get_usdc_asset(). These wrap the existing private helpers (admin, invoice_contract, escrow_contract) and include rustdoc matching the style of get_usdc_asset. Tests cover both happy-path and uninitialized-panic cases. Closes TrusTrove#578 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
|
@Bogunrot 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! 🚀 |
|
Needs changes Public getters for invoice and escrow contracts call private helpers directly without checking option unwrapping or panicking on uninitialized state, causing test failures on expected panic messages.
Reviewed commit: |
There was a problem hiding this comment.
Needs changes
Private helper implementations for invoice and escrow contract getters do not unwrap or handle Option types properly compared to admin and usdc assets.
contracts/pool/src/lib.rs:206:Self::invoice_contract(&env)returns anOption<Address>(or raw helper return type), but lacks.expect(...)likeget_adminandget_usdc_asset, causing a type mismatch or missing panic behavior required by the issue.contracts/pool/src/lib.rs:228:Self::escrow_contract(&env)lacks.expect(...)to unwrap the Option and provide an uninitialized panic message, which is tested intest.rsexpecting specific panic strings.
Reviewed commit: 458bbbad5e03142dc162558303368e41e56ad046.
…e#676) Resolve the test.rs conflict by keeping main's checks-effects-interactions tests alongside the new public-getter tests for admin, invoice contract, and escrow contract. The getters delegate to private helpers that already unwrap with clear panic messages, so no behavior change was needed. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
There was a problem hiding this comment.
Needs changes
Public getters for invoice and escrow contracts call private helpers directly without checking option unwrapping or panicking on uninitialized state, causing test failures on expected panic messages.
contracts/pool/src/lib.rs:202: get_invoice_contract calls Self::invoice_contract(&env) directly which returns Option instead of unwrapping or panicking when uninitialized, causing the corresponding test to fail.contracts/pool/src/lib.rs:224: get_escrow_contract calls Self::escrow_contract(&env) directly which returns Option instead of unwrapping or panicking when uninitialized, causing the corresponding test to fail.
Reviewed commit: 3a113939203540033cd3ede908b51d270871f2c4.
… pattern Make the private invoice_contract/escrow_contract helpers return Option<Address> (raw storage read) and move the .expect(...) into the public getters, mirroring get_admin. Internal call sites unwrap with the same messages, so panic behavior is unchanged. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
|
MergeKeeper review Scope: in scope for linked issue The pull request correctly implements the public getters for admin, invoice contract, and escrow contract along with all required tests and documentation matching the issue requirements. Reviewed commit: |
|
Merged Merged with |
…e#674) main now contains the merged public-getter work (TrusTrove#676), which touched the same pool files. Keep both: main's getter tests alongside the pool_initialized event test from this PR. All 109 pool tests pass. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
Linked Issue
Closes #578
Problem Statement (The Bug)
The pool contract stores
Admin,InvoiceContract, andEscrowContractaddresses in instance storage and has private helpers (Self::admin,Self::invoice_contract,Self::escrow_contract) to read them, but onlyUsdcAssetis exposed via a public getter (get_usdc_asset). There is no way for an off-chain client, another contract, or a test to read the pool's configured admin/invoice/escrow addresses without inspecting raw storage.Solution Comparison and Decision
env.storage().instance().get(&DataKey::Admin)directly. This leaks internal key names and breaks encapsulation.get_usdc_asset. Consistent, simple, and safe.Option B was chosen because it follows the established pattern and provides a clean public API.
The Change (Code modifications)
contracts/pool/src/lib.rsget_admin(env) -> Addresscontracts/pool/src/lib.rsget_invoice_contract(env) -> Addresscontracts/pool/src/lib.rsget_escrow_contract(env) -> Addresscontracts/pool/src/test.rsCompatibility Note (On INTERFACE_VERSION)
No interface version change. This is a purely additive change — three new public read-only entry points are added. No existing interface is modified.
Incidental Fixes
Testing (Proving it works)
test_get_admin_returns_correct_address: Verifies the admin address matches the one used ininitialize().test_get_invoice_contract_returns_correct_address: Verifies the invoice contract address matches the one used ininitialize().test_get_escrow_contract_returns_correct_address: Verifies the escrow contract address matches the one used ininitialize().test_get_admin_panics_when_uninitialized: Asserts panic for uninitialized contract.test_get_invoice_contract_panics_when_uninitialized: Asserts panic for uninitialized contract.test_get_escrow_contract_panics_when_uninitialized: Asserts panic for uninitialized contract.All 6 new tests pass.
Additional Notes
These getters mirror the existing
get_usdc_assetpattern and use the private helpers already present in the contract. No storage key changes are needed.