Integrating a Price Oracle for Condition-Based Escrow in Soroban #63
Conversation
WalkthroughThis pull request introduces a comprehensive integration of a price oracle with the escrow contract. It adds new configuration files, documentation, and guidelines while establishing a modular codebase for handling escrow operations—including funding, dispute resolution, and conditional fund release based on external price data. New modules, contracts, and tests are provided with detailed JSON snapshots to validate contract interactions. Token management functionalities and structured storage modules are also introduced, reflecting extensive enhancements to the project’s core functionality. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EscrowContract
participant PriceOracle
participant Ledger
User->>EscrowContract: Request fund release
EscrowContract->>PriceOracle: Fetch current price for asset
PriceOracle-->>EscrowContract: Return price data
EscrowContract->>Ledger: Validate price condition
Ledger-->>EscrowContract: Condition met/failed
EscrowContract->>User: Release funds (if condition met)
Assessment against linked issues
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (40)
Price-Oracle-Contract/GIT_GUIDELINE.md (4)
6-32: Good branch naming convention, but consider adding a scope sectionThe branch naming convention is well-structured with appropriate types. However, consider adding guidance on including a scope section (like
feat/escrow/add-price-oracle) to further clarify which part of the codebase is being modified.🧰 Tools
🪛 LanguageTool
[grammar] ~22-~22: You’ve repeated a verb. Did you mean to only write one of them?
Context: ...ld**: Build system changes - ci: CI changes - change: Littles changes - chore: Other c...(REPEATED_VERBS)
22-24: Fix grammatical error in branch type descriptionThere's a grammatical error in one of the branch types.
- **change**: Littles changes + **change**: Little changes🧰 Tools
🪛 LanguageTool
[grammar] ~22-~22: You’ve repeated a verb. Did you mean to only write one of them?
Context: ...ld**: Build system changes - ci: CI changes - change: Littles changes - chore: Other c...(REPEATED_VERBS)
48-49: Clarify character count referenceThe guideline mentions "less than 72 characters" but doesn't specify if this refers to the subject line only or the entire commit message. Consider clarifying this to avoid confusion.
56-56: Fix grammar in the closing noteThe closing note has a grammatical error.
- ## Thanks for follow the guidelines + ## Thanks for following the guidelinesPrice-Oracle-Contract/CONTRIBUTORS_GUIDELINE.md (8)
32-33: Fix numbering inconsistency in section headingsThere's a numbering error in the section headings. The "Create a New Branch" section is labeled as "3" but should be "4" to maintain sequence.
- ## 3. Create a New Branch + ## 4. Create a New Branch
43-44: Fix numbering inconsistency in section headingsThe "Make Atomic Commits" section numbering is incorrect.
- ## 4. Make Atomic Commits + ## 5. Make Atomic Commits
55-56: Fix numbering inconsistency in section headingsThe "Push Your Changes" section numbering is incorrect.
- ## 5. Push Your Changes + ## 6. Push Your Changes
65-66: Fix numbering inconsistency in section headingsThe "Generate a Pull Request (PR)" section numbering is incorrect.
- ## 6. Generate a Pull Request (PR) + ## 7. Generate a Pull Request (PR)
28-28: Add missing comma in sentenceThere's a missing comma in this sentence.
- If you have successfully completed the tests you are ready to start contributing. 🚀 + If you have successfully completed the tests, you are ready to start contributing. 🚀🧰 Tools
🪛 LanguageTool
[uncategorized] ~28-~28: A comma might be missing here.
Context: ... If you have successfully completed the tests you are ready to start contributing. 🚀...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
67-69: Add more details about the PR templateThe contribution guide mentions a PR template but doesn't explain where to find it or what it looks like. Consider adding more details or a link to the template.
- Follow the PR template below to submit your PR. - **Important:** If you don't use the provided PR template properly, your PR will be ignored. + Follow the PR template that will automatically appear when you create a PR. + **Important:** If you don't use the provided PR template properly, your PR will be ignored. + + For reference, the PR template includes sections for: + - Description of changes + - Issue link + - Type of change (bug fix, feature, documentation, etc.) + - Checklist of completion criteria
34-35: Update Git Guidelines link to point to repository-relative pathThe link to the Git Guidelines points to an absolute GitHub URL. Consider using a repository-relative path for better maintainability.
- Create a new branch according to the guidelines in the following document: [Git Guidelines](https://github.com/Tico4Chain-Coders/Trustless-Work/blob/main/GIT_GUIDELINE.md). + Create a new branch according to the guidelines in the following document: [Git Guidelines](../GIT_GUIDELINE.md).
45-46: Update Git Guidelines link to point to repository-relative pathThe link to the Git Guidelines points to an absolute GitHub URL. Consider using a repository-relative path for better maintainability.
- Create atomic commits following the guidelines outlined here: [Git Guidelines](https://github.com/Tico4Chain-Coders/Trustless-Work/blob/main/GIT_GUIDELINE.md). + Create atomic commits following the guidelines outlined here: [Git Guidelines](../GIT_GUIDELINE.md).Price-Oracle-Contract/Cargo.toml (1)
21-21: Maintain consistent language in documentation.The comment is in Spanish while the rest of the codebase appears to be in English. Consider translating it to English for consistency.
- # For more información sobre este perfil ve https://soroban.stellar.org/docs/basic-tutorials/logging#cargotoml-profile + # For more information about this profile see https://soroban.stellar.org/docs/basic-tutorials/logging#cargotoml-profilePrice-Oracle-Contract/contracts/engagement/src/storage/store.rs (1)
3-11: Consider adding additional derives for the enum.For contract storage types, it's often useful to add derives for
Debug,Eq, andPartialEqin addition toClone.-#[derive(Clone)] +#[derive(Clone, Debug, Eq, PartialEq)] #[contracttype] enum DataKeyAddress { Initialized, TotalAddress, Shares(u32), Addresses(u32), }Price-Oracle-Contract/contracts/engagement/Cargo.toml (1)
1-5: Consider updating the version number from 0.0.0.The package version is set to 0.0.0, which indicates early development. As this PR integrates price oracle functionality, consider setting a more appropriate version number that reflects the maturity of the contract.
Price-Oracle-Contract/contracts/engagement/src/error.rs (1)
48-134: Error message formatting is clear and consistentThe implementation of the Display trait for ContractError provides clear, descriptive error messages that will be helpful for debugging. The price oracle related errors (FailedToFetchPrice and EscrowAlreadyReleased) have appropriate messages.
However, there's a spelling inconsistency in some of the error messages:
- ContractError::OnlyServiceProviderChangeMilstoneStatus => { - write!(f, "Only the service provider can change milestone status") + ContractError::OnlyServiceProviderChangeMilestoneStatus => { + write!(f, "Only the service provider can change milestone status") } - ContractError::NoMileStoneDefined => write!(f, "Escrow initialized without milestone"), - ContractError::InvalidMileStoneIndex => write!(f, "Invalid milestone index"), - ContractError::OnlyApproverChangeMilstoneFlag => { - write!(f, "Only the approver can change milestone flag") + ContractError::NoMilestoneDefined => write!(f, "Escrow initialized without milestone"), + ContractError::InvalidMilestoneIndex => write!(f, "Invalid milestone index"), + ContractError::OnlyApproverChangeMilestoneFlag => { + write!(f, "Only the approver can change milestone flag")Note: You'll need to update the enum variant names as well, not just the error messages.
Price-Oracle-Contract/contracts/engagement/src/core/admin.rs (1)
19-30: Rename error for clarity.Currently, returning
ContractError::AdminNotFoundwhen the newly stored admin doesn’t match the just-written address might cause confusion. Consider introducing a more specific error (e.g.,AdminWriteFailed) if the stored address unexpectedly differs.- return Err(ContractError::AdminNotFound); + return Err(ContractError::AdminWriteFailed);Price-Oracle-Contract/contracts/engagement/src/core/dispute.rs (1)
120-137: Require authentication for toggling dispute status.
change_dispute_flagcan be called by anyone under the current logic, which might allow unauthorized users to mark an escrow as disputed. If intended, no action needed; otherwise, consider restricting access (e.g., requiring the service provider or dispute resolver to authorize).Price-Oracle-Contract/contracts/engagement/src/core/price_oracle.rs (2)
15-29: Improve error handling in fetch_price methodThe method currently uses
panic!for error conditions, which abruptly terminates contract execution. Consider using more graceful error handling with proper return types to allow callers to handle errors.- pub fn fetch_price(env: &Env, asset: Asset) -> PriceData { + pub fn fetch_price(env: &Env, asset: Asset) -> Result<PriceData, Error> { let reflector_contract_id = match env.storage().instance().get(&oracle_key) { Some(x) => x, - None => panic!("Please_initialize_oracle"), + None => return Err(Error::OracleNotInitialized), }; let reflector_contract = Client::new(env, &reflector_contract_id); let asset_price: Option<PriceData> = reflector_contract.lastprice(&asset); match asset_price { Some(x) => x, - None => panic!("failed_to_fetch_price"), + None => return Err(Error::PriceFetchFailed), } }This would require defining an Error enum in your crate:
#[derive(Clone, Debug, Eq, PartialEq)] pub enum Error { OracleNotInitialized, PriceFetchFailed, }
1-4: Consider adding proper error and type documentationThe file lacks documentation for the imported types and defined functions. Adding proper documentation would improve code maintainability and usability.
+/// This module implements a price oracle contract that integrates with the reflector oracle. +/// It provides functionality to initialize the oracle, fetch asset prices, and check price conditions. use crate::core::reflector_oracle::{Asset, Client, PriceData}; use crate::storage::types::oracle_key; use soroban_sdk::{contract, contractimpl, Address, Env}; +/// The PriceOracle contract provides price data for assets by interacting with an external oracle. #[contract] pub struct PriceOracle;Price-Oracle-Contract/contracts/engagement/test_snapshots/test/test_claim_escrow_earnings_successful_flow.1.json (1)
1-1840: Comprehensive JSON Snapshot for Successful Earnings ClaimThis new JSON test snapshot is detailed and well-structured. It accurately represents the generators, authentication data, ledger entries, and event flows for a successful claim escrow earnings scenario. To further improve long‐term maintainability, consider providing an accompanying schema description or inline comments (perhaps in a separate documentation file) that explain the purpose of each major section or key group.
Price-Oracle-Contract/contracts/engagement/test_snapshots/test/test_claim_escrow_earnings_milestones_incomplete.1.json (1)
1-774: JSON Snapshot for Incomplete Milestones ScenarioThis JSON snapshot effectively captures the ledger state and events for an escrow scenario where milestones remain incomplete. The structure and key-value pairs mirror the expected smart contract interactions. As with other JSON snapshots, consider adding either inline comments or a separate schema reference guide to clarify the purpose of nested objects and keys for future maintainers.
Price-Oracle-Contract/contracts/engagement/test_snapshots/test/test_claim_escrow_earnings_no_milestones.1.json (2)
280-350: Validate the failed call scenario.
The"failed_call": truefields indicate an error scenario. Verify that the test covers both the successful and failing paths for escrow functions, ensuring full coverage of potential contract states.Consider adding a complementary snapshot with a successful call to verify normal function flow.
586-612: Ensure error handling is documented.
Lines 586–612 reveal an error path forclaim_escrow_earnings. Consider documenting how the contract or calling code recovers from or logs these errors.Updating test documentation or readme notes about these error states can improve maintainability.
Price-Oracle-Contract/contracts/engagement/src/core/reflector_oracle.rs (1)
6-51: Add documentation for each trait method.
The contract trait is extensive, but lacks doc comments explaining usage, expected inputs, and return values.Add Rust-style doc comments (
/// ...) to each method to make it easier for future maintainers or integrators.Price-Oracle-Contract/contracts/engagement/src/core/milestone.rs (1)
10-33: Consider adding event logs for milestone status changes.
While you do publish the final escrow state, explicitly logging the milestone status change in an event can simplify debugging and off-chain tracking.for (index, milestone) in existing_escrow.milestones.iter().enumerate() { let mut new_milestone = milestone.clone(); if index as i128 == milestone_index { new_milestone.status = new_status.clone(); + // Example event logging + e.events().publish( + ("milestone_status_changed",), + (milestone_index, new_status.clone()), + ); } updated_milestones.push_back(new_milestone); }Price-Oracle-Contract/contracts/engagement/src/tests/test.rs (3)
22-22: Correct the test function name to avoid typos.The function name is spelled "test_initialize_excrow" instead of "test_initialize_escrow," which may cause confusion or inconsistency across the codebase.
-fn test_initialize_excrow() { +fn test_initialize_escrow() {
105-105: Avoid floating-point arithmetic for fee calculations.Casting a floating result into
i128can cause subtle rounding issues. Consider using an integer-based approach for accuracy.- let platform_fee = (0.3 * 10i128.pow(18) as f64) as i128; + // Using a purely integer approach for 0.3 * 10^18: + let platform_fee = 300_000_000_000_000_000i128;
21-1049: Abstract repeated test setup to improve maintainability.Multiple tests repeat logic for environment setup, address generation, and escrow property creation. Extract these into a helper function or test fixture to reduce duplication and enhance clarity.
Price-Oracle-Contract/contracts/engagement/src/token/token.rs (3)
12-16: Consider returning a contract error instead of panicking.
Thecheck_nonnegative_amountfunction usespanic!for negative values. Implementing a custom error or reusing a structured error enum aligns better with robust error handling.fn check_nonnegative_amount(amount: i128) { if amount < 0 { - panic!("negative amount is not allowed: {}", amount); + // Example: Convert to a specialized error + panic!("NegativeAmountError: {}", amount); } }
18-41: Validate token naming constraints.
Theinitializelogic is clear, but consider ensuringnameandsymbolare not empty strings. You may also want to throw a structured error if these conditions fail, rather than panicking.
145-161: Fallback to panic on missing metadata keys.
decimals,name, andsymbolpanic if storage keys are missing. Consider returning a custom error or using a safe default.fn symbol(e: Env) -> String { match e.storage().instance().get(&"symbol") { Some(symbol) => symbol, - None => panic!("The 'symbol' key was not found in storage"), + None => panic!("SymbolNotFoundError"), } }Price-Oracle-Contract/contracts/engagement/src/token/allowance.rs (1)
23-52: Strong validations inwrite_allowance.
The code ensuresexpiration_ledgeris valid whenamount > 0and extends TTL properly. One minor suggestion: consider returning an error enum instead of panicking ifexpiration_ledger < ledger.sequence().Price-Oracle-Contract/contracts/engagement/src/core/escrow.rs (1)
224-275: Conditional fund release with price check.
Therelease_fundsmethod checks an external oracle price condition and then transfers funds. Implementation is correct, but consider clarifying what happens if the price condition is not met (there’s no explicit error or fallback).Price-Oracle-Contract/contracts/engagement/src/storage/types.rs (2)
6-6: Maintain uppercase naming convention for constants.
oracle_keyis aconstbut uses lowercase naming. For consistency and clarity, consider naming itORACLE_KEY.-pub(crate) const oracle_key: Symbol = symbol_short!("oracle"); +pub(crate) const ORACLE_KEY: Symbol = symbol_short!("oracle");
22-24: Use specialized data representation for escrow state.
Multiple boolean fields (dispute_flag,release_flag,resolved_flag) may be clearer if replaced with an enum representing the escrow’s overall state. This improves code readability by preventing incompatible flags from being set simultaneously (e.g.,release_flaganddispute_flagat once).Price-Oracle-Contract/contracts/engagement/src/contract.rs (3)
14-33: Add explicit error handling for contract invocation.
Theinvoke_contractcall returns aValwithout verifying success or failure at the remote contract. Consider handling errors or unexpected return values more robustly, especially if your logic depends on the remote contract’s state changes.- let res: Val = env.invoke_contract(&deployed_address, &init_fn, init_args); + let invocation_result: Val = env.invoke_contract(&deployed_address, &init_fn, init_args); + // Optionally parse or check the invocation_result here to confirm success
78-81: Correct the spelling of “platform_address.”
“plataform_address” may cause confusion or hamper searchability in the codebase. Consider renaming to maintain clarity and consistency.- pub fn change_escrow_properties(e: Env, plataform_address: Address, ... + pub fn change_escrow_properties(e: Env, platform_address: Address, ...
144-162: Optional: Expand test coverage for dispute resolution methods.
Although the dispute logic is defined, verifying it with a robust suite of tests, including edge cases (e.g., negative funds, large values, repeated calls), ensures better reliability.Do you want me to generate additional tests for
resolving_disputesto strengthen coverage?Price-Oracle-Contract/PRICE_ORACLE.md (1)
272-280: Remove hard tabs and replace with spaces to satisfy markdownlint checks.
Markdownlint has flagged lines with indentation tabs in your Makefile snippet. Please convert them to spaces to ensure a consistent formatting style.- test: build - cargo test - stellar contract build - @ls -l target/wasm32-unknown-unknown/release/*.wasm - stellar snapshot create --network mainnet --output json ... + test: build + cargo test + stellar contract build + @ls -l target/wasm32-unknown-unknown/release/*.wasm + stellar snapshot create --network mainnet --output json ...🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
272-272: Hard tabs
Column: 1(MD010, no-hard-tabs)
275-275: Hard tabs
Column: 1(MD010, no-hard-tabs)
276-276: Hard tabs
Column: 1(MD010, no-hard-tabs)
279-279: Hard tabs
Column: 1(MD010, no-hard-tabs)
280-280: Hard tabs
Column: 1(MD010, no-hard-tabs)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
Price-Oracle-Contract/Cargo.lockis excluded by!**/*.lockPrice-Oracle-Contract/contracts/.DS_Storeis excluded by!**/.DS_StorePrice-Oracle-Contract/contracts/engagement/.DS_Storeis excluded by!**/.DS_Store
📒 Files selected for processing (39)
Price-Oracle-Contract/.gitignore(1 hunks)Price-Oracle-Contract/CONTRIBUTORS_GUIDELINE.md(1 hunks)Price-Oracle-Contract/Cargo.toml(1 hunks)Price-Oracle-Contract/GIT_GUIDELINE.md(1 hunks)Price-Oracle-Contract/PRICE_ORACLE.md(1 hunks)Price-Oracle-Contract/README.md(1 hunks)Price-Oracle-Contract/contracts/engagement/Cargo.toml(1 hunks)Price-Oracle-Contract/contracts/engagement/src/contract.rs(1 hunks)Price-Oracle-Contract/contracts/engagement/src/core/admin.rs(1 hunks)Price-Oracle-Contract/contracts/engagement/src/core/dispute.rs(1 hunks)Price-Oracle-Contract/contracts/engagement/src/core/escrow.rs(1 hunks)Price-Oracle-Contract/contracts/engagement/src/core/milestone.rs(1 hunks)Price-Oracle-Contract/contracts/engagement/src/core/mod.rs(1 hunks)Price-Oracle-Contract/contracts/engagement/src/core/price_oracle.rs(1 hunks)Price-Oracle-Contract/contracts/engagement/src/core/reflector_oracle.rs(1 hunks)Price-Oracle-Contract/contracts/engagement/src/error.rs(1 hunks)Price-Oracle-Contract/contracts/engagement/src/events/handler.rs(1 hunks)Price-Oracle-Contract/contracts/engagement/src/events/mod.rs(1 hunks)Price-Oracle-Contract/contracts/engagement/src/lib.rs(1 hunks)Price-Oracle-Contract/contracts/engagement/src/storage/mod.rs(1 hunks)Price-Oracle-Contract/contracts/engagement/src/storage/store.rs(1 hunks)Price-Oracle-Contract/contracts/engagement/src/storage/types.rs(1 hunks)Price-Oracle-Contract/contracts/engagement/src/tests/mod.rs(1 hunks)Price-Oracle-Contract/contracts/engagement/src/tests/price_oracle_test.rs(1 hunks)Price-Oracle-Contract/contracts/engagement/src/tests/test.rs(1 hunks)Price-Oracle-Contract/contracts/engagement/src/token/allowance.rs(1 hunks)Price-Oracle-Contract/contracts/engagement/src/token/balance.rs(1 hunks)Price-Oracle-Contract/contracts/engagement/src/token/metadata.rs(1 hunks)Price-Oracle-Contract/contracts/engagement/src/token/mod.rs(1 hunks)Price-Oracle-Contract/contracts/engagement/src/token/token.rs(1 hunks)Price-Oracle-Contract/contracts/engagement/test_snapshots/test/test.1.json(1 hunks)Price-Oracle-Contract/contracts/engagement/test_snapshots/test/test_change_escrow_properties.1.json(1 hunks)Price-Oracle-Contract/contracts/engagement/test_snapshots/test/test_claim_escrow_earnings_milestones_incomplete.1.json(1 hunks)Price-Oracle-Contract/contracts/engagement/test_snapshots/test/test_claim_escrow_earnings_no_milestones.1.json(1 hunks)Price-Oracle-Contract/contracts/engagement/test_snapshots/test/test_claim_escrow_earnings_successful_flow.1.json(1 hunks)Price-Oracle-Contract/contracts/engagement/test_snapshots/test/test_client_can_recover_funds_if_service_provider_does_not_complete_all_escrows.1.json(1 hunks)Price-Oracle-Contract/contracts/engagement/test_snapshots/test/test_create_fund_complete_escrows.1.json(1 hunks)Price-Oracle-Contract/contracts/engagement/test_snapshots/test/test_dispute_flag_management.1.json(1 hunks)Price-Oracle-Contract/contracts/engagement/test_snapshots/test/test_dispute_resolution_flow.1.json(1 hunks)
🧰 Additional context used
🧬 Code Definitions (6)
Price-Oracle-Contract/contracts/engagement/src/events/mod.rs (1)
Price-Oracle-Contract/contracts/engagement/src/events/handler.rs (1)
escrows_by_engagement_id(5-13)
Price-Oracle-Contract/contracts/engagement/src/token/mod.rs (1)
Price-Oracle-Contract/contracts/engagement/src/token/token.rs (2)
allowance(60-65)balance(82-87)
Price-Oracle-Contract/contracts/engagement/src/tests/test.rs (2)
Price-Oracle-Contract/contracts/engagement/src/core/escrow.rs (2)
e(206-206)e(216-219)Price-Oracle-Contract/contracts/engagement/src/token/token.rs (1)
balance(82-87)
Price-Oracle-Contract/contracts/engagement/src/token/token.rs (4)
Price-Oracle-Contract/contracts/engagement/src/core/admin.rs (3)
has_administrator(6-9)read_administrator(11-17)write_administrator(19-30)Price-Oracle-Contract/contracts/engagement/src/token/allowance.rs (4)
read_allowance(4-21)spend_allowance(54-66)write_allowance(23-52)e(6-6)Price-Oracle-Contract/contracts/engagement/src/token/balance.rs (4)
read_balance(8-18)receive_balance(28-35)spend_balance(37-50)e(10-10)Price-Oracle-Contract/contracts/engagement/src/token/metadata.rs (1)
write_metadata(4-7)
Price-Oracle-Contract/contracts/engagement/src/core/escrow.rs (2)
Price-Oracle-Contract/contracts/engagement/src/contract.rs (7)
initialize_escrow(39-45)fund_escrow(47-58)get_escrow(96-98)distribute_escrow_earnings(60-76)change_escrow_properties(78-94)get_multiple_escrow_balances(107-112)get_escrow_by_contract_id(100-105)Price-Oracle-Contract/contracts/engagement/src/core/price_oracle.rs (1)
checks_price_condition(31-39)
Price-Oracle-Contract/contracts/engagement/src/core/milestone.rs (3)
Price-Oracle-Contract/contracts/engagement/src/events/handler.rs (1)
escrows_by_engagement_id(5-13)Price-Oracle-Contract/contracts/engagement/src/contract.rs (3)
change_milestone_status(122-129)get_escrow(96-98)change_milestone_flag(131-138)Price-Oracle-Contract/contracts/engagement/src/core/escrow.rs (3)
e(206-206)e(216-219)get_escrow(215-222)
🪛 LanguageTool
Price-Oracle-Contract/CONTRIBUTORS_GUIDELINE.md
[uncategorized] ~28-~28: A comma might be missing here.
Context: ... If you have successfully completed the tests you are ready to start contributing. 🚀...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
Price-Oracle-Contract/GIT_GUIDELINE.md
[grammar] ~22-~22: You’ve repeated a verb. Did you mean to only write one of them?
Context: ...ld**: Build system changes - ci: CI changes - change: Littles changes - chore: Other c...
(REPEATED_VERBS)
[uncategorized] ~35-~35: Loose punctuation mark.
Context: ...styling ### Types of Commits -feat: A new feature - fix: A bug fix - doc...
(UNLIKELY_OPENING_PUNCTUATION)
[grammar] ~36-~36: “Bug” is a singular noun. It appears that the verb form is incorrect.
Context: ... - feat: A new feature - fix: A bug fix - docs: Documentation changes - `styl...
(PCT_SINGULAR_NOUN_PLURAL_VERB_AGREEMENT)
[uncategorized] ~39-~39: Loose punctuation mark.
Context: ...the code (formatting, etc.) - refactor: Code changes that neither fix a bug nor...
(UNLIKELY_OPENING_PUNCTUATION)
Price-Oracle-Contract/README.md
[style] ~200-~200: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...r own account). - <path_to_wasm_file> is the path to the .wasm file generated ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~212-~212: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...nally, to deploy the contract, you will need to use the output from the previous comman...
(REP_NEED_TO_VB)
[style] ~238-~238: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... deployment will be made. - <network> is the network you are working on (e.g., t...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.17.2)
Price-Oracle-Contract/README.md
73-73: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
89-89: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
95-95: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
102-102: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
138-138: Multiple spaces after hash on atx style heading
null
(MD019, no-multiple-space-atx)
204-204: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
214-214: Bare URL used
null
(MD034, no-bare-urls)
216-216: Multiple headings with the same content
null
(MD024, no-duplicate-heading)
225-225: Multiple headings with the same content
null
(MD024, no-duplicate-heading)
Price-Oracle-Contract/PRICE_ORACLE.md
272-272: Hard tabs
Column: 1
(MD010, no-hard-tabs)
275-275: Hard tabs
Column: 1
(MD010, no-hard-tabs)
276-276: Hard tabs
Column: 1
(MD010, no-hard-tabs)
279-279: Hard tabs
Column: 1
(MD010, no-hard-tabs)
280-280: Hard tabs
Column: 1
(MD010, no-hard-tabs)
🔇 Additional comments (47)
Price-Oracle-Contract/.gitignore (1)
1-5: LGTM: Standard .gitignore entries for Rust/Soroban projectThe .gitignore file properly excludes the standard directories that should not be tracked in version control:
target/for Rust build artifacts.soroban/for local Soroban configurationThese are appropriate exclusions for a Soroban contract project.
Price-Oracle-Contract/contracts/engagement/src/events/handler.rs (1)
11-12: Verify that the event payload structure is consistent with other eventsEnsure that the event payload structure follows a consistent pattern across your contract events for easier client-side filtering and handling. Consider documenting the event structure in a separate documentation file for client developers.
Price-Oracle-Contract/contracts/engagement/src/storage/mod.rs (1)
1-2: Module structure looks clean and well-organized.The code establishes a clear modular structure for storage concerns by exposing two public modules -
storefor data access/persistence andtypesfor data structure definitions. This separation of concerns follows good Rust practices and will help maintain a clean architecture as the oracle integration grows.Price-Oracle-Contract/contracts/engagement/src/token/metadata.rs (1)
4-7: Token metadata writing implementation looks good.The function provides a clean wrapper around the Soroban token SDK's metadata functionality. It correctly initializes the
TokenUtilsinstance with the environment reference and delegates to the appropriate SDK method.Price-Oracle-Contract/contracts/engagement/src/events/mod.rs (1)
1-3: Event handling structure looks appropriate.Good organization of event-related functionality with module encapsulation and crate-level visibility for the handler function. The restricted visibility using
pub(crate)maintains proper encapsulation while allowing internal modules to publish escrow events.Price-Oracle-Contract/Cargo.toml (2)
11-19: LGTM! Optimized release profile for contract deployment.The release profile is properly optimized for contract deployment with size optimization, proper security settings, and Link Time Optimization enabled.
8-9:Details
❓ Verification inconclusive
Verify Soroban SDK versioning compatibility.
The project is using soroban-sdk version 21.0.0 and soroban-token-sdk version 21.6.0. Make sure these versions are compatible with each other and with your target Stellar network.
🏁 Script executed:
#!/bin/bash # Check for compatibility issues between these versions curl -s https://github.com/stellar/soroban-sdk/releases | grep -A 5 "21.0.0" curl -s https://github.com/stellar/soroban-token-sdk/releases | grep -A 5 "21.6.0"Length of output: 159
Action Required: Confirm Dependency Versions and Standardize Comment Language
The workspace configuration in
Price-Oracle-Contract/Cargo.tomlcorrectly pins the dependencies tosoroban-sdk = "21.0.0"andsoroban-token-sdk = { version = "21.6.0" }, which aligns with common practices for Soroban projects. However, our initial automated check using GitHub release pages did not return any output to validate these versions.
- Dependency Verification: Please manually verify on GitHub that these specific versions are compatible with your target Stellar network and intended SDK functionalities.
- Comment Language: Also consider standardizing the comment language—currently, there’s a Spanish comment amidst English text—to maintain consistency.
Price-Oracle-Contract/contracts/engagement/src/token/mod.rs (1)
1-4: LGTM! Well-organized token module structure.The token module is well-organized with a clear separation of concerns between allowance, balance, metadata, and core token functionality. This structure will support the escrow contract's need to handle tokens based on price oracle conditions.
Price-Oracle-Contract/contracts/engagement/Cargo.toml (2)
7-8: LGTM! Correct crate type for Soroban contract.The crate-type is properly set to cdylib, which is required for Soroban smart contracts to be compiled into WebAssembly and deployed to the Stellar network.
10-16: LGTM! Dependencies correctly reference workspace versions.Dependencies are properly configured to use workspace-level versions, ensuring consistency across the project. The testutils feature is correctly enabled for dev-dependencies, which is essential for contract testing.
Price-Oracle-Contract/contracts/engagement/test_snapshots/test/test.1.json (1)
1-132:Details
❓ Verification inconclusive
Test snapshot file structure looks good, but contains empty contract code
This JSON file provides a well-structured test snapshot for blockchain contract testing, including generators, auth settings, ledger configuration, and event records. However, I noticed that the contract code hash (line 54) appears to be the SHA-256 hash of an empty string, and the code field (line 64) is empty.
While this might be intentional for test purposes, please confirm that this empty contract code is the intended behavior for this test snapshot.
🏁 Script executed:
#!/bin/bash # Verify if this is the SHA-256 hash of an empty string echo -n "" | sha256sumLength of output: 124
Action Required: Confirm Intentional Empty Contract Code
The snapshot file (
Price-Oracle-Contract/contracts/engagement/test_snapshots/test/test.1.json) is well-structured overall. However, note that the contract code hash on line 54 is set to the SHA‑256 hash of an empty string (i.e. “e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855”) and the corresponding"code"field on line 64 is empty.Given that the initial SHA‑256 verification using
sha256sumfailed due to the missing command, please manually verify (or run an alternative verification, e.g. using a Python script) that:
- The hash indeed corresponds to an empty string.
- The empty contract code is intentional for this test snapshot.
Price-Oracle-Contract/contracts/engagement/src/error.rs (1)
4-46: Well-structured error handling with oracle-specific errorsThe ContractError enum is comprehensive and includes specific errors for the price oracle integration, which is aligned with the PR objective. The enum definition follows best practices with sequential numbering.
Price-Oracle-Contract/contracts/engagement/src/lib.rs (1)
1-11: Well-organized module structure for the price oracle integrationThe library structure is clean and follows good practices:
- Using
#![no_std]is appropriate for smart contracts to minimize dependencies- The modules are logically separated by functionality
- The code organization facilitates the integration of the price oracle with the escrow system
Price-Oracle-Contract/contracts/engagement/test_snapshots/test/test_change_escrow_properties.1.json (1)
1-2864: Consider verifying snapshot integrity and ensuring consistency with contract updates.This JSON test snapshot references multiple addresses, engagement IDs, milestone updates, and contract calls. Please confirm the correctness of:
- All contract addresses, particularly for "CAAAAAAAAAA..." placeholders.
- Argument consistency for
change_escrow_propertiesacross repeated calls.- Proper handling of success vs. failed calls and alignment with the updated dispute logic.
If these entries are final placeholders, proceed; otherwise, consider inserting valid or more descriptive references.
Would you like a script to parse and validate each engagement ID, contract address, and event output to ensure consistency with your contract’s updated schema?
Price-Oracle-Contract/contracts/engagement/src/core/admin.rs (2)
6-9: Looks good.The
has_administratorfunction is straightforward and appropriately checks for a stored admin.
11-17: Ensure test coverage for missing admin scenario.Returning
ContractError::AdminNotFoundis a clean approach if no admin is configured. Please confirm that the contract’s tests cover this case (e.g., verifying the error result when no admin is written).Would you like help generating additional test cases to validate this scenario thoroughly?
Price-Oracle-Contract/contracts/engagement/test_snapshots/test/test_dispute_resolution_flow.1.json (1)
1-1790: Test snapshots validate dispute resolution flowThe test snapshot properly demonstrates the dispute resolution flow, including initialization of an escrow, changing dispute flags, and error handling when unauthorized users attempt to change dispute flags.
Price-Oracle-Contract/contracts/engagement/test_snapshots/test/test_create_fund_complete_escrows.1.json (1)
1-2349:Details
❌ Incorrect review comment
Test snapshots cover escrow lifecycle but need price oracle tests
The test snapshot comprehensively covers the escrow lifecycle including creation, funding, completion, and earnings distribution. However, there appear to be no tests demonstrating integration with the newly added price oracle functionality.
Consider adding tests specifically for the price oracle integration to ensure the escrow contract can properly interact with price data for condition-based releases. Here's a script to check if such tests exist:
🏁 Script executed:
#!/bin/bash # Look for test files related to price oracle echo "Searching for price oracle tests..." find Price-Oracle-Contract/contracts -type f -name "*test*.rs" -o -name "*test*.json" | xargs grep -l "price_oracle\|PriceOracle" || echo "No price oracle tests found" echo "Checking contract integration with price oracle..." find Price-Oracle-Contract/contracts -type f -name "*.rs" | xargs grep -l "checks_price_condition" || echo "No references to price condition checks found in contracts"Length of output: 687
Heads-up: Price Oracle Tests Already Exist
The test snapshots thoroughly cover the escrow lifecycle, and dedicated tests verifying price oracle integration are already present. For example, the repository includes tests in
• Price-Oracle-Contract/contracts/engagement/src/tests/price_oracle_test.rs and
• Price-Oracle-Contract/contracts/engagement/src/tests/test.rs.
Additionally, integration points in modules like escrow.rs and price_oracle.rs confirm that the price-based condition checks are properly exercised. No additional tests are necessary.Likely an incorrect or invalid review comment.
Price-Oracle-Contract/README.md (1)
1-244: Comprehensive and Informative READMEThe README provides detailed instructions on installation, configuration, and deployment for the Price Oracle Contract. The structure is clear and the content is useful for both newcomers and experienced developers. There are, however, a few minor markdown style issues reported by static analysis that you may want to address:
- Header Formatting: Several headings (e.g., on lines 73, 89, 95, and 102) contain trailing punctuation. Removing these endings (such as periods or colons) will enhance the professional look and consistency.
- Duplicate Headings: There are repeated headings with similar text (lines 216–225). Consider rephrasing or consolidating them to improve navigation.
- Excessive Spacing: On line 138, extra spaces following the hash markers should be removed for consistency.
- Code Block Language: Ensure that all fenced code blocks include a language identifier (e.g.,
bash) as suggested on line 204.- Bare URLs: Replace any bare URLs (such as on line 214) with proper markdown links to improve readability.
Overall, the document is excellent and with these minor adjustments it will be even clearer.
🧰 Tools
🪛 LanguageTool
[style] ~200-~200: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...r own account). -<path_to_wasm_file>is the path to the.wasmfile generated ...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~212-~212: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...nally, to deploy the contract, you will need to use the output from the previous comman...(REP_NEED_TO_VB)
[style] ~238-~238: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... deployment will be made. -<network>is the network you are working on (e.g., t...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.17.2)
73-73: Trailing punctuation in heading
Punctuation: '.'(MD026, no-trailing-punctuation)
89-89: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
95-95: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
102-102: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
138-138: Multiple spaces after hash on atx style heading
null(MD019, no-multiple-space-atx)
204-204: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
214-214: Bare URL used
null(MD034, no-bare-urls)
216-216: Multiple headings with the same content
null(MD024, no-duplicate-heading)
225-225: Multiple headings with the same content
null(MD024, no-duplicate-heading)
Price-Oracle-Contract/contracts/engagement/src/core/mod.rs (1)
1-13: Clean and Modular Core Module SetupThe new module declarations and re-export statements are straightforward and well-organized. This modular design (including modules for admin, dispute, escrow, milestone, price oracle, and reflector oracle) enhances the maintainability and scalability of the codebase.
Price-Oracle-Contract/contracts/engagement/test_snapshots/test/test_claim_escrow_earnings_no_milestones.1.json (1)
44-50:Details
✅ Verification successful
Confirm the zero-length WASM code usage.
An empty WASM code hash (e3b0...b855) indicates a zero-length code. Ensure this is intentional for your testing scenario.Run a quick search to confirm no other test references a non-empty code hash for the same contract scenario:
🏁 Script executed:
#!/bin/bash # Search for references of zero-length WASM code usage in test snapshots rg 'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855' -A 5 -B 5Length of output: 73969
Review Update – Zero-Length WASM Code Usage Confirmed
The empty WASM hash
e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855
has been consistently referenced across multiple test snapshot files (e.g., in the "test_distribute_escrow_earnings_no_milestones", "test_initialize_time_based_excrow", "test_initialize_manual_excrow", and several others). This confirms the usage of a zero-length WASM code is intentional for the testing scenarios.Actionable Points:
- Verify that all tests expecting a “no WASM code” scenario (mocking purposes) are configured correctly.
- If any tests need to simulate non-empty WASM code behavior, ensure they are handled separately.
- Note: A connection reset was logged for
test_initialize_escrow_prices_cannot_be_zero.1.json. Please verify that this error is unrelated to the intentional use of the zero-length hash.Overall, the pattern observed confirms the intentional use of zero-length WASM code.
Price-Oracle-Contract/contracts/engagement/src/core/reflector_oracle.rs (1)
52-86:Details
❓ Verification inconclusive
Raise potential concurrency concerns and usage guidelines.
Configurations (likeconfigmethod) can drastically affect the oracle’s state if called by multiple admin addresses. Ensure concurrency or reentrancy concerns are addressed at the contract level.Check throughout the code for any concurrency or reentrancy guards:
🏁 Script executed:
#!/bin/bash # Searching for concurrency/reentrancy keywords or relevant patterns rg "(reentrancy|concurrency|require_auth|mutex|lock)" -A 5 -B 2Length of output: 72102
Attention: Potential Concurrency and Reentrancy Risks in Admin Configuration
The current implementation for state types and configuration (e.g., in the admin's
configmethod) does not show any explicit locks or reentrancy guards. Our grep for keywords like “reentrancy,” “concurrency,” “mutex,” and “lock” turned up no relevant safeguards in the contract code. Since multiple admin addresses could potentially call configuration methods concurrently, please verify that:
- The contract’s execution model (or underlying Soroban runtime) provides sufficient isolation to prevent state corruption from simultaneous updates.
- Any admin operations that modify critical state are either inherently atomic or are augmented with explicit concurrency/reentrancy guards.
- Usage guidelines are clearly documented, so callers understand the safe order and constraints for configuration updates.
Please review and confirm the concurrency handling strategy to ensure robust protection against potential race conditions.
Price-Oracle-Contract/contracts/engagement/test_snapshots/test/test_dispute_flag_management.1.json (1)
1-1747: Data file looks good.This JSON snapshot effectively captures ledger events and states. No issues or security concerns detected in the stored data.
Price-Oracle-Contract/contracts/engagement/src/token/balance.rs (1)
8-18: Implementation ofread_balanceis clear and concise.Reading the balance from persistent storage and bumping TTL ensures updated ledger retention. No concerns here.
Price-Oracle-Contract/contracts/engagement/src/token/token.rs (9)
1-11: Good use of Soroban SDK and token interfaces.
Imports and module setup look consistent. No concerns here.
43-55: Zero-amount minting check.
If zero-amount minting is disallowed or should be handled differently, add a check or a custom error. Otherwise, the current approach is acceptable.
60-65: Allowance retrieval logic is consistent.
Extending the instance storage TTL is a good practice. No issues found.
67-80: Approval procedure is straightforward.
Authorization, nonnegative check, and event logging are properly handled. Looks good.
82-87: Balance query function is concise.
This is a standard approach for retrieving balances. No concerns.
89-101: Transfer behavior is correct.
Authorization, TTL extension, and corresponding event emission look correct. The flow of spending and receiving balances matches typical token transfers.
103-116: Allowance-based transfer is well-implemented.
You properly ensure the spender has authorization and then spend from allowance before transferring. Good alignment with ERC-20 style patterns.
118-129: Burn function is properly guarded.
Authorization, negative checks, and event emission are handled. No immediate concerns.
131-143: Burn from function works as intended.
Spender-based burn logic mirrors allowance usage and is consistent with the rest of the token interface.Price-Oracle-Contract/contracts/engagement/src/token/allowance.rs (1)
1-21: Allowance check for expired ledgers is appropriate.
Theread_allowancefunction resets the amount to zero if expired; this is a safe approach to disallow usage of stale allowances.Price-Oracle-Contract/contracts/engagement/src/core/escrow.rs (7)
12-26: Initialization logic is correct.
The function checks for an existing escrow, ensures the amount is nonzero, then saves. No obvious errors.
28-64: Potential partial deposit scenario.
Thefund_escrowfunction ensures there's no dispute and checks that the contract is not already fully funded. However, line 51 checks if the contract balance is greater than the entire escrow amount, returningEscrowFullyFundedif it is. Consider verifying partial funding logic (e.g., multiple smaller deposits) to avoid overfunding.
66-148: Distribution process is comprehensive.
You check:
- Milestones completed
- Dispute not flagged
- Contract has enough balance
- Then distribute to multiple parties
Implementation is thorough. Watch out for integer overflows with large amounts, though you handle it withchecked_operations. No further major issues.
150-172: Change escrow properties controlled by platform.
Requires platform address auth. This approach is valid. No concerns.
174-198: Bulk balance retrieval is convenient.
get_multiple_escrow_balancesloops through addresses and fetches each escrow's trustline. Implementation is straightforward.
200-213:get_escrow_by_contract_idusesinvoke_contract.
This pattern can be beneficial but requires the callee to provide aget_escrowfunction. Ensure error handling is robust since a misconfigured contract could break.
215-222: Graceful fallback for missing escrow.
CollectingEscrowNotFoundfrom storage is consistent. This helps avoid panic.Price-Oracle-Contract/contracts/engagement/src/storage/types.rs (1)
3-5: Consider verifying these constants align with your ledger frequency.
DAY_IN_LEDGERSis set to 17280, presumably corresponding to a 5-second ledger close. Ensure this assumption remains correct throughout the contract’s lifecycle; otherwise, values such asINSTANCE_BUMP_AMOUNTorINSTANCE_LIFETIME_THRESHOLDmight be misaligned.Would you like a script to confirm that this frequency matches the current Soroban ledger parameters?
Price-Oracle-Contract/contracts/engagement/src/contract.rs (1)
50-51: Validate the deposit amount to avoid negative or zero deposits.
Add input checks onamount_to_depositto ensure you don’t inadvertently accept invalid deposits that might lead to unexpected escrow balances.Would you like a script to search for method calls to
fund_escrowand confirm they always pass valid amounts?Price-Oracle-Contract/contracts/engagement/test_snapshots/test/test_client_can_recover_funds_if_service_provider_does_not_complete_all_escrows.1.json (4)
1-5: Generators Section: Valid and Concise
The"generators"block correctly establishes the test generator configuration with"address": 8and"nonce": 0. Ensure that these static values match the expected setup in your test framework and remain consistent with any upstream test configuration changes.
6-118: Auth Section: Comprehensive Invocation Structure
This section captures multiple contract invocations (e.g., formint,fund_escrow, andrefund_remaining_funds) along with nested sub-invocations. The structure is detailed and appears to adhere to the expected schema. Please verify that every contract address, function name, and argument (such as the usage of the engagement id"41431") accurately reflects the updated escrow functionality and the integration of the price oracle.
119-650: Ledger Section: Detailed State Snapshot
The"ledger"segment provides a granular snapshot of the contract’s state, including entries for both temporary and persistent data (e.g., via"ledger_key_nonce","vec", and"ledger_key_contract_instance"). It is essential to confirm that these entries accurately mirror the contract’s state post-oracle integration. In particular, check that numeric fields like the"i128"representations and nonce values align with your expectations.
651-2130: Events Section: Extensive and Informative Logging
This section logs a wide range of events—diagnostic and contract events—for operations includingmint,initialize,fund_escrow,cancel_escrow,transfer, andrefund_remaining_funds. The detailed structure with topics, nested data, and return events is comprehensive. Ensure that all event topics and corresponding data values (addresses, amounts, status strings, etc.) are updated to reflect the new conditional release logic driven by the integrated price oracle. Maintaining tight synchronization between these snapshots and the evolving contract logic (especially with respect to oracle-driven triggers) will be key to reliable testing.
| pub fn escrows_by_engagement_id(e: &Env, engagement_id: String, escrow: Escrow) { | ||
| let topics = (symbol_short!("p_by_spdr"),); | ||
|
|
||
| let engagement_id_val: Val = engagement_id.into_val(e); | ||
| let escrow_val: Val = escrow.into_val(e); | ||
|
|
||
| let event_payload = vec![e, engagement_id_val, escrow_val]; | ||
| e.events().publish(topics, event_payload); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve event name clarity and add documentation
The current event name "p_by_spdr" is not self-descriptive. Consider using a more explicit name like "escrow_by_engagement" and adding function documentation to explain the purpose of this event.
- // ------ Escrows
+ /// Publishes an event that associates an escrow with an engagement ID
+ ///
+ /// This event is emitted when an escrow is created or updated for a specific engagement
+ /// and can be used by clients to track the relationship between engagements and escrows.
+ ///
+ /// # Parameters
+ /// * `e` - The Soroban environment
+ /// * `engagement_id` - The ID of the engagement
+ /// * `escrow` - The escrow object that is associated with the engagement
pub fn escrows_by_engagement_id(e: &Env, engagement_id: String, escrow: Escrow) {
- let topics = (symbol_short!("p_by_spdr"),);
+ let topics = (symbol_short!("escrow_by_engagement"),);
let engagement_id_val: Val = engagement_id.into_val(e);
let escrow_val: Val = escrow.into_val(e);
let event_payload = vec![e, engagement_id_val, escrow_val];
e.events().publish(topics, event_payload);
}Committable suggestion skipped: line range outside the PR's diff.
| #[cfg(test)] | ||
| mod test; | ||
| mod price_oracle_test; |
There was a problem hiding this comment.
Module configuration needs adjustment.
The #[cfg(test)] attribute only applies to the test module while price_oracle_test isn't conditionally compiled. This may cause issues if production code tries to use the price_oracle_test module.
Apply this fix:
#[cfg(test)]
mod test;
+#[cfg(test)]
mod price_oracle_test;Committable suggestion skipped: line range outside the PR's diff.
| // pub fn get_all_engagements(e: Env) -> Vec<Escrow> { | ||
| // let engagement_count: u128 = e | ||
| // .storage() | ||
| // .instance() | ||
| // .get(&symbol_short!("pk")) | ||
| // .unwrap_or(0); | ||
|
|
||
| // let mut engagements: Vec<Engagement> = Vec::new(&e); | ||
|
|
||
| // for id in 1..=engagement_count { | ||
| // let engagement_id_in_bytes = u128_to_bytes(&e, id); | ||
| // let engagement_key = DataKey::Engagement(engagement_id_in_bytes); | ||
| // if let Some(engagement) = e.storage().instance().get(&engagement_key) { | ||
| // engagements.push_back(engagement); | ||
| // } | ||
| // } | ||
|
|
||
| // engagements | ||
| // } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Resolve inconsistent commented code.
The commented-out function get_all_engagements references types and functions not defined in this file (DataKey::Engagement, u128_to_bytes, Escrow, Engagement), and doesn't use the DataKeyAddress enum defined above. Consider either:
- Updating the commented function to use the defined
DataKeyAddressenum - Removing the commented code if it's not needed
- Implementing the missing references if this function will be used
🏁 Script executed:
#!/bin/bash
# Check if the referenced types exist elsewhere in the codebase
echo "Searching for DataKey, Escrow, and Engagement types..."
rg "enum DataKey" --type rust
rg "struct Escrow" --type rust
rg "struct Engagement" --type rust
rg "fn u128_to_bytes" --type rustLength of output: 1235
Action Required: Resolve inconsistencies in the commented-out function
The commented-out function get_all_engagements references:
DataKey::Engagementwhile the codebase definesDataKeyAddressin this module.u128_to_bytesis missing from the repository.Engagementis undefined (only anEngagementContractis found), even thoughEscrowexists.
Please either update the commented function to use the correct references and defined types or remove it if it’s no longer needed.
| #[test] | ||
| fn test_price_oracle() { | ||
| let mut env = Env::from_ledger_snapshot_file("../../../../snapshot.json"); | ||
| env.set_config(EnvTestConfig { | ||
| capture_snapshot_at_drop: false, | ||
| }); | ||
|
|
||
| let contract_id = env.register_contract(None, PriceOracle); | ||
| let client = PriceOracleClient::new(&env, &contract_id); | ||
|
|
||
| let reflector_address = Address::from_string(&String::from_str( | ||
| &env, | ||
| "CAVLP5DH2GJPZMVO7IJY4CVOD5MWEFTJFVPD2YY2FQXOQHRGHK4D6HLP", | ||
| )); | ||
|
|
||
| client.initialize_oracle(&reflector_address); | ||
|
|
||
| let asset = &Asset::Other(symbol_short!("BTC")); | ||
| let price = client.fetch_price(asset); | ||
|
|
||
| println!("{:?}", price.price); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Test function lacks assertions and error handling
The test initializes and calls the price oracle successfully, but has several issues:
- It references a snapshot file at a relative path without verifying its existence
- It prints the price but doesn't assert any expected values
- It doesn't test error conditions or edge cases
Consider enhancing the test with:
#[test]
fn test_price_oracle() {
- let mut env = Env::from_ledger_snapshot_file("../../../../snapshot.json");
+ // Use a mock environment instead of relying on external file
+ let mut env = Env::default();
env.set_config(EnvTestConfig {
capture_snapshot_at_drop: false,
});
let contract_id = env.register_contract(None, PriceOracle);
let client = PriceOracleClient::new(&env, &contract_id);
let reflector_address = Address::from_string(&String::from_str(
&env,
"CAVLP5DH2GJPZMVO7IJY4CVOD5MWEFTJFVPD2YY2FQXOQHRGHK4D6HLP",
));
client.initialize_oracle(&reflector_address);
let asset = &Asset::Other(symbol_short!("BTC"));
let price = client.fetch_price(asset);
- println!("{:?}", price.price);
+ // Add assertions to verify the price is as expected
+ assert!(price.price > 0, "Price should be greater than zero");
+
+ // Test error condition: Try fetching with unintialized oracle
+ let client2 = PriceOracleClient::new(&env, &env.register_contract(None, PriceOracle));
+ let result = std::panic::catch_unwind(|| {
+ client2.fetch_price(asset);
+ });
+ assert!(result.is_err(), "Should fail when oracle is not initialized");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[test] | |
| fn test_price_oracle() { | |
| let mut env = Env::from_ledger_snapshot_file("../../../../snapshot.json"); | |
| env.set_config(EnvTestConfig { | |
| capture_snapshot_at_drop: false, | |
| }); | |
| let contract_id = env.register_contract(None, PriceOracle); | |
| let client = PriceOracleClient::new(&env, &contract_id); | |
| let reflector_address = Address::from_string(&String::from_str( | |
| &env, | |
| "CAVLP5DH2GJPZMVO7IJY4CVOD5MWEFTJFVPD2YY2FQXOQHRGHK4D6HLP", | |
| )); | |
| client.initialize_oracle(&reflector_address); | |
| let asset = &Asset::Other(symbol_short!("BTC")); | |
| let price = client.fetch_price(asset); | |
| println!("{:?}", price.price); | |
| } | |
| #[test] | |
| fn test_price_oracle() { | |
| // Use a mock environment instead of relying on external file | |
| let mut env = Env::default(); | |
| env.set_config(EnvTestConfig { | |
| capture_snapshot_at_drop: false, | |
| }); | |
| let contract_id = env.register_contract(None, PriceOracle); | |
| let client = PriceOracleClient::new(&env, &contract_id); | |
| let reflector_address = Address::from_string(&String::from_str( | |
| &env, | |
| "CAVLP5DH2GJPZMVO7IJY4CVOD5MWEFTJFVPD2YY2FQXOQHRGHK4D6HLP", | |
| )); | |
| client.initialize_oracle(&reflector_address); | |
| let asset = &Asset::Other(symbol_short!("BTC")); | |
| let price = client.fetch_price(asset); | |
| // Add assertions to verify the price is as expected | |
| assert!(price.price > 0, "Price should be greater than zero"); | |
| // Test error condition: Try fetching with unintialized oracle | |
| let client2 = PriceOracleClient::new(&env, &env.register_contract(None, PriceOracle)); | |
| let result = std::panic::catch_unwind(|| { | |
| client2.fetch_price(asset); | |
| }); | |
| assert!(result.is_err(), "Should fail when oracle is not initialized"); | |
| } |
| pub fn resolving_disputes( | ||
| e: Env, | ||
| dispute_resolver: Address, | ||
| approver_funds: i128, | ||
| service_provider_funds: i128, | ||
| trustless_work_address: Address, | ||
| ) -> Result<(), ContractError> { | ||
| dispute_resolver.require_auth(); | ||
|
|
||
| let escrow_result = EscrowManager::get_escrow(e.clone()); | ||
| let mut escrow = match escrow_result { | ||
| Ok(esc) => esc, | ||
| Err(err) => return Err(err), | ||
| }; | ||
|
|
||
| if dispute_resolver != escrow.dispute_resolver { | ||
| return Err(ContractError::OnlyDisputeResolverCanExecuteThisFunction); | ||
| } | ||
|
|
||
| if !escrow.dispute_flag { | ||
| return Err(ContractError::EscrowNotInDispute); | ||
| } | ||
|
|
||
| let usdc_approver = TokenClient::new(&e, &escrow.trustline); | ||
| let escrow_balance = usdc_approver.balance(&e.current_contract_address()); | ||
|
|
||
| let total_funds = approver_funds | ||
| .checked_add(service_provider_funds) | ||
| .ok_or(ContractError::Overflow)?; | ||
| if total_funds > escrow_balance { | ||
| return Err(ContractError::InsufficientFundsForResolution); | ||
| } | ||
|
|
||
| let trustless_work_fee = total_funds | ||
| .checked_mul(30) | ||
| .ok_or(ContractError::Overflow)? | ||
| .checked_div(10000) | ||
| .ok_or(ContractError::DivisionError)?; | ||
| let platform_fee = total_funds | ||
| .checked_mul(escrow.platform_fee) | ||
| .ok_or(ContractError::Overflow)? | ||
| .checked_div(10000) | ||
| .ok_or(ContractError::DivisionError)?; | ||
| let total_fees = trustless_work_fee | ||
| .checked_add(platform_fee) | ||
| .ok_or(ContractError::Overflow)?; | ||
|
|
||
| let approver_fee = approver_funds | ||
| .checked_mul(total_fees) | ||
| .ok_or(ContractError::Overflow)? | ||
| .checked_div(total_funds) | ||
| .ok_or(ContractError::DivisionError)?; | ||
| let net_approver_funds = approver_funds | ||
| .checked_sub(approver_fee) | ||
| .ok_or(ContractError::Underflow)?; | ||
| let fees_portion = service_provider_funds | ||
| .checked_mul(total_fees) | ||
| .ok_or(ContractError::Overflow)? | ||
| .checked_div(total_funds) | ||
| .ok_or(ContractError::DivisionError)?; | ||
| let net_provider_funds = service_provider_funds | ||
| .checked_sub(fees_portion) | ||
| .ok_or(ContractError::Underflow)?; | ||
|
|
||
| if approver_funds < net_approver_funds { | ||
| return Err(ContractError::InsufficientApproverFundsForCommissions); | ||
| } | ||
|
|
||
| if service_provider_funds < net_provider_funds { | ||
| return Err(ContractError::InsufficientServiceProviderFundsForCommissions); | ||
| } | ||
|
|
||
| usdc_approver.transfer( | ||
| &e.current_contract_address(), | ||
| &trustless_work_address, | ||
| &trustless_work_fee, | ||
| ); | ||
|
|
||
| usdc_approver.transfer( | ||
| &e.current_contract_address(), | ||
| &escrow.platform_address, | ||
| &platform_fee, | ||
| ); | ||
|
|
||
| if net_approver_funds > 0 { | ||
| usdc_approver.transfer( | ||
| &e.current_contract_address(), | ||
| &escrow.approver, | ||
| &net_approver_funds, | ||
| ); | ||
| } | ||
|
|
||
| if net_provider_funds > 0 { | ||
| usdc_approver.transfer( | ||
| &e.current_contract_address(), | ||
| &escrow.service_provider, | ||
| &net_provider_funds, | ||
| ); | ||
| } | ||
|
|
||
| escrow.resolved_flag = true; | ||
| e.storage().instance().set(&DataKey::Escrow, &escrow); | ||
|
|
||
| escrows_by_engagement_id(&e, escrow.engagement_id.clone(), escrow); | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate non-negative funds to prevent unexpected outcomes.
Consider enforcing that approver_funds and service_provider_funds must be non-negative. Negative inputs could lead to unintended arithmetic results. Verifying input ranges or returning an error if values are below zero would safeguard against underflows and logical inconsistencies.
| pub fn checks_price_condition( | ||
| env: Env, | ||
| asset: Asset, | ||
| target_price: i128, | ||
| amount: i128, | ||
| ) -> bool { | ||
| let current_price = Self::fetch_price(&env, asset); | ||
| current_price.price * amount >= target_price | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for input parameters in checks_price_condition
The checks_price_condition method doesn't validate the input parameters. Negative values for target_price or amount could lead to unexpected results.
pub fn checks_price_condition(
env: Env,
asset: Asset,
target_price: i128,
amount: i128,
) -> bool {
+ // Validate input parameters
+ if amount <= 0 {
+ panic!("Amount must be positive");
+ }
+
let current_price = Self::fetch_price(&env, asset);
- current_price.price * amount >= target_price
+ // Handle potential overflow in multiplication
+ match current_price.price.checked_mul(amount) {
+ Some(product) => product >= target_price,
+ None => panic!("Price calculation overflow")
+ }
}Additionally, if you implement the error handling suggestion for fetch_price, you'd need to update this method to handle those errors as well.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn checks_price_condition( | |
| env: Env, | |
| asset: Asset, | |
| target_price: i128, | |
| amount: i128, | |
| ) -> bool { | |
| let current_price = Self::fetch_price(&env, asset); | |
| current_price.price * amount >= target_price | |
| } | |
| pub fn checks_price_condition( | |
| env: Env, | |
| asset: Asset, | |
| target_price: i128, | |
| amount: i128, | |
| ) -> bool { | |
| // Validate input parameters | |
| if amount <= 0 { | |
| panic!("Amount must be positive"); | |
| } | |
| let current_price = Self::fetch_price(&env, asset); | |
| // Handle potential overflow in multiplication | |
| match current_price.price.checked_mul(amount) { | |
| Some(product) => product >= target_price, | |
| None => panic!("Price calculation overflow") | |
| } | |
| } |
| pub fn change_milestone_flag( | ||
| e: Env, | ||
| milestone_index: i128, | ||
| new_flag: bool, | ||
| approver: Address, | ||
| ) -> Result<(), ContractError> { | ||
| let escrow_result = EscrowManager::get_escrow(e.clone()); | ||
| let existing_escrow = match escrow_result { | ||
| Ok(esc) => esc, | ||
| Err(err) => return Err(err), | ||
| }; | ||
|
|
||
| if approver != existing_escrow.approver { | ||
| return Err(ContractError::OnlyApproverChangeMilstoneFlag); | ||
| } | ||
|
|
||
| approver.require_auth(); | ||
|
|
||
| if existing_escrow.milestones.is_empty() { | ||
| return Err(ContractError::NoMileStoneDefined); | ||
| } | ||
|
|
||
| if milestone_index < 0 || milestone_index >= existing_escrow.milestones.len() as i128 { | ||
| return Err(ContractError::InvalidMileStoneIndex); | ||
| } | ||
|
|
||
| let mut updated_milestones = Vec::<Milestone>::new(&e); | ||
| for (index, milestone) in existing_escrow.milestones.iter().enumerate() { | ||
| let mut new_milestone = milestone.clone(); | ||
| if index as i128 == milestone_index { | ||
| new_milestone.approved_flag = new_flag; | ||
| } | ||
| updated_milestones.push_back(new_milestone); | ||
| } | ||
|
|
||
| let updated_escrow = Escrow { | ||
| milestones: updated_milestones, | ||
| ..existing_escrow | ||
| }; | ||
|
|
||
| e.storage() | ||
| .instance() | ||
| .set(&DataKey::Escrow, &updated_escrow); | ||
|
|
||
| escrows_by_engagement_id(&e, updated_escrow.engagement_id.clone(), updated_escrow); | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against out-of-sequence calls when changing milestone flags.
If a milestone is incomplete or not yet in the proper stage, setting approved_flag may cause confusion. Consider an additional check for milestone status before allowing an approval flag change.
| pub fn receive_balance(e: &Env, addr: Address, amount: i128) { | ||
| let balance = read_balance(e, addr.clone()); | ||
| let total_balance = match balance.checked_add(amount) { | ||
| Some(sum) => sum, | ||
| None => panic!("Overflow when adding {} to balance {}", amount, balance), | ||
| }; | ||
| write_balance(e, addr, total_balance); | ||
| } | ||
|
|
||
| pub fn spend_balance(e: &Env, addr: Address, amount: i128) { | ||
| let balance = read_balance(e, addr.clone()); | ||
| if balance < amount { | ||
| panic!("insufficient balance"); | ||
| } | ||
| let total_balance = match balance.checked_sub(amount) { | ||
| Some(diff) => diff, | ||
| None => panic!( | ||
| "Underflow when subtracting {} from balance {}", | ||
| amount, balance | ||
| ), | ||
| }; | ||
| write_balance(e, addr, total_balance); | ||
| } |
There was a problem hiding this comment.
Validate non-negative amount to prevent unintended balance manipulation.
Currently, a negative amount passed to receive_balance or spend_balance can yield incorrect results or security issues. Ensure that amounts are greater than zero to mitigate misuse.
pub fn receive_balance(e: &Env, addr: Address, amount: i128) {
+ if amount < 0 {
+ panic!("`amount` cannot be negative");
+ }
let balance = read_balance(e, addr.clone());
...
pub fn spend_balance(e: &Env, addr: Address, amount: i128) {
+ if amount <= 0 {
+ panic!("`amount` must be greater than 0");
+ }
let balance = read_balance(e, addr.clone());
...📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn receive_balance(e: &Env, addr: Address, amount: i128) { | |
| let balance = read_balance(e, addr.clone()); | |
| let total_balance = match balance.checked_add(amount) { | |
| Some(sum) => sum, | |
| None => panic!("Overflow when adding {} to balance {}", amount, balance), | |
| }; | |
| write_balance(e, addr, total_balance); | |
| } | |
| pub fn spend_balance(e: &Env, addr: Address, amount: i128) { | |
| let balance = read_balance(e, addr.clone()); | |
| if balance < amount { | |
| panic!("insufficient balance"); | |
| } | |
| let total_balance = match balance.checked_sub(amount) { | |
| Some(diff) => diff, | |
| None => panic!( | |
| "Underflow when subtracting {} from balance {}", | |
| amount, balance | |
| ), | |
| }; | |
| write_balance(e, addr, total_balance); | |
| } | |
| pub fn receive_balance(e: &Env, addr: Address, amount: i128) { | |
| if amount < 0 { | |
| panic!("`amount` cannot be negative"); | |
| } | |
| let balance = read_balance(e, addr.clone()); | |
| let total_balance = match balance.checked_add(amount) { | |
| Some(sum) => sum, | |
| None => panic!("Overflow when adding {} to balance {}", amount, balance), | |
| }; | |
| write_balance(e, addr, total_balance); | |
| } | |
| pub fn spend_balance(e: &Env, addr: Address, amount: i128) { | |
| if amount <= 0 { | |
| panic!("`amount` must be greater than 0"); | |
| } | |
| let balance = read_balance(e, addr.clone()); | |
| if balance < amount { | |
| panic!("insufficient balance"); | |
| } | |
| let total_balance = match balance.checked_sub(amount) { | |
| Some(diff) => diff, | |
| None => panic!( | |
| "Underflow when subtracting {} from balance {}", | |
| amount, balance | |
| ), | |
| }; | |
| write_balance(e, addr, total_balance); | |
| } |
| pub fn spend_allowance(e: &Env, from: Address, spender: Address, amount: i128) { | ||
| let allowance = read_allowance(e, from.clone(), spender.clone()); | ||
| if allowance.amount < amount { | ||
| panic!("insufficient allowance"); | ||
| } | ||
| if amount > 0 { | ||
| if let Some(_new_amount) = allowance.amount.checked_sub(amount) { | ||
| write_allowance(e, from, spender, amount, allowance.expiration_ledger); | ||
| } else { | ||
| panic!("Underflow: subtraction failed in spend_allowance"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Spending allowance logic is consistent.
Use of checked_sub avoids underflow, and leftover allowance is updated accordingly. Ensure the newly updated allowance is indeed the old allowance minus the spent amount, rather than the amount alone, at line 61.
Are you certain you intended to call write_allowance(e, from, spender, amount, ...) instead of the new subtracted amount? Let's confirm:
🏁 Script executed:
#!/bin/bash
# Searching for spend_allowance usage to see if there's alternative references.
rg -A 5 'spend_allowance'Length of output: 5303
Fix required: Incorrect allowance update
The subtraction with checked_sub correctly computes the new allowance value, but the call to write_allowance is still using the spent amount rather than this new value. Please update the function so that the allowance is reduced by the spent amount. For example, modify the code as follows:
pub fn spend_allowance(e: &Env, from: Address, spender: Address, amount: i128) {
let allowance = read_allowance(e, from.clone(), spender.clone());
if allowance.amount < amount {
panic!("insufficient allowance");
}
if amount > 0 {
if let Some(new_amount) = allowance.amount.checked_sub(amount) {
write_allowance(e, from, spender, new_amount, allowance.expiration_ledger);
} else {
panic!("Underflow: subtraction failed in spend_allowance");
}
}
}Please verify that this behavior aligns with the intended logic.
| use soroban_sdk::{symbol_short, testutils::EnvTestConfig, Address, Env, String}; | ||
|
|
||
| #[test] | ||
| fntest_price_oracle() { |
There was a problem hiding this comment.
Fix function declaration syntax for the test.
It appears to read fntest_price_oracle() instead of a standard Rust function declaration fn test_price_oracle(). This will fail to compile.
- fntest_price_oracle() {
+ fn test_price_oracle() {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fntest_price_oracle() { | |
| fn test_price_oracle() { |
2 participants
close #54
Modifiction to escrow contract to:
Summary by CodeRabbit
New Features
Documentation
Tests