i made the fix to issue 204: implement escrow comment/note storage#214
i made the fix to issue 204: implement escrow comment/note storage#214winnpxl wants to merge 1 commit intoOlufunbiIK:mainfrom
Conversation
📝 WalkthroughWalkthroughThe PR extends the split-escrow contract with note management capabilities. It adds error variants to the Error enum, introduces public get_note and set_note APIs, modifies create_split to accept an optional note and deadline parameter, emits note events, stores notes in escrow records, and relocates the Error enum to a dedicated errors module. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/split-escrow/src/lib.rs`:
- Around line 304-322: The set_note path currently only checks
EscrowStatus::Active but must also block updates past the escrow deadline; add a
timestamp check using the contract env (e.g., env.ledger().timestamp() or
env.now()) after fetching escrow (from storage::get_escrow) and before allowing
the update: if the current timestamp is greater than escrow.deadline return an
error (introduce or use an appropriate Error variant such as
Error::EscrowExpired), then continue with escrow.creator.require_auth() and the
existing status check/validate_note_length calls.
- Around line 73-75: The create_split function currently accepts any deadline
(u64) including past timestamps; add validation in create_split to reject
deadlines <= current block timestamp by retrieving the current time (e.g.,
env::block_timestamp() or equivalent) and returning an Err or panicking with a
clear message when deadline is not in the future; specifically check the
deadline parameter early in create_split (before constructing the Split/escrow)
and ensure tests/handlers that call create_split reflect the new error behavior.
- Around line 9-17: The Error enum in errors.rs is missing 12 variants
referenced elsewhere; add the following variants to the Error enum:
SplitNotFound, SplitCancelled, NoFundsAvailable, EscrowNotRefundable,
InvalidBridgeStatus, OracleNotAuthorized, VerificationNotFound, BridgeNotFound,
PriceSubmissionInvalid, ProofInvalid, SplitNotFunded, and SplitReleased, each
with unique discriminant values (continue the existing sequence used in
errors.rs to avoid collisions) so references to Error::SplitNotFound etc.
resolve; update the enum declaration in errors.rs (the Error type) to include
these discriminants and run a compile to ensure no duplicate values.
- Around line 134-150: The escrow keys were migrated to String but several
functions still use or reference u64/undefined split_id variables; update the
access paths to consistently use String keys: add a helper
split_id_key(split_id: u64) -> String (or accept String inputs) and use it
wherever escrows are stored or fetched (storage::set_escrow, storage::get_escrow
calls); in particular change deposit() to convert its u64 split_id param to a
String key before lookup, make release_funds() use its split_id_str parameter
everywhere (remove references to undefined split_id), and modify
release_funds_internal() to accept a String key (or convert the incoming String
to the expected key type) so callers pass the String split_id_str; ensure all
lookups/sets use the same String key variable (split_id_str or
split_id_key(...)) to fix the type/undefined errors.
In `@contracts/split-escrow/src/types.rs`:
- Around line 459-460: Remove the duplicate attributes by keeping a single
#[contracttype] and a single #[derive(Clone, Debug)] on the SplitEscrow struct
declaration (remove the redundant repeated attributes), and in the create_escrow
function initialize the newly added note: String field when constructing
SplitEscrow (set note to the appropriate value, e.g., an empty string or the
passed-in note variable) so the struct literal in create_escrow includes note
and the code compiles.
- Around line 492-493: Several SplitEscrow struct literals are missing the new
required field note: String; update every construction site (notably the
create_escrow() function and the test cases in test.rs) to include a note value
(e.g., an empty string or context-appropriate text) when instantiating
SplitEscrow, matching how lib.rs already supplies note; ensure all SplitEscrow
{...} literals include note and re-run compilation/tests.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
contracts/split-escrow/src/errors.rscontracts/split-escrow/src/events.rscontracts/split-escrow/src/lib.rscontracts/split-escrow/src/types.rs
| mod errors; | ||
|
|
||
| #[cfg(test)] | ||
| mod test; | ||
|
|
||
| use crate::types::{ | ||
| SplitEscrow, EscrowParticipant, EscrowStatus, Error, | ||
| SplitEscrow, EscrowParticipant, EscrowStatus, | ||
| }; | ||
| use crate::errors::Error; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, check if the files exist and examine the error enum definition
echo "=== Checking errors.rs ==="
if [ -f "contracts/split-escrow/src/errors.rs" ]; then
cat "contracts/split-escrow/src/errors.rs"
else
echo "File not found: contracts/split-escrow/src/errors.rs"
fiRepository: OlufunbiIK/StellarSplit
Length of output: 482
🏁 Script executed:
#!/bin/bash
# Check lib.rs imports and sample error references
echo "=== Checking lib.rs lines 1-50 ==="
if [ -f "contracts/split-escrow/src/lib.rs" ]; then
head -50 "contracts/split-escrow/src/lib.rs" | cat -n
else
echo "File not found"
fiRepository: OlufunbiIK/StellarSplit
Length of output: 1914
🏁 Script executed:
#!/bin/bash
# Run the verification script from the review comment
python - <<'PY'
import re
from pathlib import Path
root = Path("contracts/split-escrow/src")
lib = (root / "lib.rs").read_text()
errs = (root / "errors.rs").read_text()
refs = set(re.findall(r'Error::([A-Za-z_][A-Za-z0-9_]*)', lib))
decl = set(re.findall(r'^\s*([A-Za-z_][A-Za-z0-9_]*)\s*=', errs, re.M))
missing = sorted(refs - decl)
print("Referenced in lib.rs:", len(refs))
print("Declared in errors.rs:", len(decl))
print("\nReferenced variants:", sorted(refs))
print("\nDeclared variants:", sorted(decl))
print("\nMissing variants from errors.rs:", missing)
PYRepository: OlufunbiIK/StellarSplit
Length of output: 946
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import re
from pathlib import Path
lib = Path("contracts/split-escrow/src/lib.rs").read_text()
# Find all Error:: usages with line numbers
lines = lib.split('\n')
for i, line in enumerate(lines, 1):
if 'Error::' in line:
print(f"Line {i}: {line.strip()}")
PYRepository: OlufunbiIK/StellarSplit
Length of output: 1120
🏁 Script executed:
#!/bin/bash
# Count how many functions would be affected
echo "=== Functions returning Err(Error::...) ==="
rg "Error::(SplitNotFound|SplitCancelled|NoFundsAvailable|BridgeNotFound|SplitNotFunded|SplitReleased|VerificationNotFound|InvalidBridgeStatus|EscrowNotRefundable|OracleNotAuthorized|PriceSubmissionInvalid|ProofInvalid)" contracts/split-escrow/src/lib.rs -B 5 | head -100Repository: OlufunbiIK/StellarSplit
Length of output: 3637
Add 12 missing Error enum variants to compile successfully.
The contract references Error::SplitNotFound, Error::SplitCancelled, Error::NoFundsAvailable, Error::EscrowNotRefundable, Error::InvalidBridgeStatus, Error::OracleNotAuthorized, Error::VerificationNotFound, Error::BridgeNotFound, Error::PriceSubmissionInvalid, Error::ProofInvalid, Error::SplitNotFunded, and Error::SplitReleased at 23 locations across multiple functions, but contracts/split-escrow/src/errors.rs does not define these variants. Add all 12 missing variants to the Error enum with appropriate discriminant values.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/split-escrow/src/lib.rs` around lines 9 - 17, The Error enum in
errors.rs is missing 12 variants referenced elsewhere; add the following
variants to the Error enum: SplitNotFound, SplitCancelled, NoFundsAvailable,
EscrowNotRefundable, InvalidBridgeStatus, OracleNotAuthorized,
VerificationNotFound, BridgeNotFound, PriceSubmissionInvalid, ProofInvalid,
SplitNotFunded, and SplitReleased, each with unique discriminant values
(continue the existing sequence used in errors.rs to avoid collisions) so
references to Error::SplitNotFound etc. resolve; update the enum declaration in
errors.rs (the Error type) to include these discriminants and run a compile to
ensure no duplicate values.
| deadline: u64, | ||
| note: Option<String>, | ||
| ) -> u64 { |
There was a problem hiding this comment.
Validate deadline at split creation.
create_split accepts any deadline, including timestamps in the past, which can create immediately expired escrows.
Proposed fix
pub fn create_split(
@@
note: Option<String>,
) -> u64 {
// Verify the creator is authorizing this call
creator.require_auth();
@@
if storage::is_paused(&env) {
panic!("Contract is paused");
}
+
+ if deadline <= env.ledger().timestamp() {
+ panic!("Deadline must be in the future");
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/split-escrow/src/lib.rs` around lines 73 - 75, The create_split
function currently accepts any deadline (u64) including past timestamps; add
validation in create_split to reject deadlines <= current block timestamp by
retrieving the current time (e.g., env::block_timestamp() or equivalent) and
returning an Err or panicking with a clear message when deadline is not in the
future; specifically check the deadline parameter early in create_split (before
constructing the Split/escrow) and ensure tests/handlers that call create_split
reflect the new error behavior.
| let split_id_str = String::from_str(&env, &split_id.to_string()); | ||
|
|
||
| let escrow = SplitEscrow { | ||
| split_id: split_id.clone(), | ||
| split_id: split_id_str.clone(), | ||
| creator: creator.clone(), | ||
| requester: creator.clone(), | ||
| description: description.clone(), | ||
| total_amount, | ||
| amount_collected: 0, | ||
| participants, | ||
| status: EscrowStatus::Active, | ||
| deadline: deadline, | ||
| deadline, | ||
| created_at: env.ledger().timestamp(), | ||
| note: note_value.clone(), | ||
| }; | ||
|
|
||
| storage::set_escrow(&env, &split_id, &escrow); | ||
| storage::set_escrow(&env, &split_id_str, &escrow); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the file and check its structure
git ls-files | grep -E "(split-escrow|lib\.rs)"Repository: OlufunbiIK/StellarSplit
Length of output: 1557
🏁 Script executed:
# Let's examine the file and count total lines
wc -l contracts/split-escrow/src/lib.rsRepository: OlufunbiIK/StellarSplit
Length of output: 104
🏁 Script executed:
# Now inspect the split_id usage patterns as suggested
rg -n -C2 'String::from_str\(&env, &split_id.to_string\(\)\)|pub fn deposit\(|split_id: u64|get_escrow\(&env, &split_id_str\)|set_escrow\(&env, &split_id_str' contracts/split-escrow/src/lib.rsRepository: OlufunbiIK/StellarSplit
Length of output: 3929
🏁 Script executed:
# Also search more broadly for split_id usage in the file
rg -n 'split_id' contracts/split-escrow/src/lib.rs | head -40Repository: OlufunbiIK/StellarSplit
Length of output: 2805
Fix undefined variable and type mismatches in split_id key migration.
The code stores escrows with String keys (line 150) but the deposit() function tries to access them using the undefined variable split_id_str (line 174), which doesn't exist in that function's scope. Additionally, at line 242, split_id_str (a String) is passed to release_funds_internal() which expects a u64, causing a type mismatch. Similarly, release_funds() receives split_id_str as a parameter but line 256 references undefined split_id.
This breaks compilation. Convert all escrow access paths to consistently use the String key form:
- In
deposit(): convert theu64 split_idparameter toStringbefore accessing escrows - In
release_funds(): use thesplit_id_strparameter consistently instead of undefinedsplit_id - In
release_funds_internal(): acceptStringkeys or convert them appropriately for escrow lookups
A helper function like the suggested split_id_key() would prevent this drift across all access paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/split-escrow/src/lib.rs` around lines 134 - 150, The escrow keys
were migrated to String but several functions still use or reference
u64/undefined split_id variables; update the access paths to consistently use
String keys: add a helper split_id_key(split_id: u64) -> String (or accept
String inputs) and use it wherever escrows are stored or fetched
(storage::set_escrow, storage::get_escrow calls); in particular change deposit()
to convert its u64 split_id param to a String key before lookup, make
release_funds() use its split_id_str parameter everywhere (remove references to
undefined split_id), and modify release_funds_internal() to accept a String key
(or convert the incoming String to the expected key type) so callers pass the
String split_id_str; ensure all lookups/sets use the same String key variable
(split_id_str or split_id_key(...)) to fix the type/undefined errors.
| pub fn set_note(env: Env, split_id: String, note: String) -> Result<(), Error> { | ||
| if storage::is_paused(&env) { | ||
| panic!("Contract is paused"); | ||
| } | ||
|
|
||
| // Validate note length | ||
| Self::validate_note_length(¬e)?; | ||
|
|
||
| // Get escrow | ||
| let mut escrow = storage::get_escrow(&env, &split_id) | ||
| .ok_or(Error::EscrowNotFound)?; | ||
|
|
||
| // Verify caller is the creator | ||
| escrow.creator.require_auth(); | ||
|
|
||
| // Check escrow status is Active | ||
| if escrow.status != EscrowStatus::Active { | ||
| return Err(Error::InvalidEscrowStatus); | ||
| } |
There was a problem hiding this comment.
set_note should reject updates after deadline, not only by status.
If EscrowStatus is still Active past deadline (lazy state update), note edits are still allowed. Add a timestamp check before permitting update.
Proposed fix
pub fn set_note(env: Env, split_id: String, note: String) -> Result<(), Error> {
@@
let mut escrow = storage::get_escrow(&env, &split_id)
.ok_or(Error::EscrowNotFound)?;
@@
// Verify caller is the creator
escrow.creator.require_auth();
+
+ if env.ledger().timestamp() > escrow.deadline {
+ escrow.status = EscrowStatus::Expired;
+ storage::set_escrow(&env, &split_id, &escrow);
+ return Err(Error::EscrowExpired);
+ }
// Check escrow status is Active
if escrow.status != EscrowStatus::Active {
return Err(Error::InvalidEscrowStatus);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/split-escrow/src/lib.rs` around lines 304 - 322, The set_note path
currently only checks EscrowStatus::Active but must also block updates past the
escrow deadline; add a timestamp check using the contract env (e.g.,
env.ledger().timestamp() or env.now()) after fetching escrow (from
storage::get_escrow) and before allowing the update: if the current timestamp is
greater than escrow.deadline return an error (introduce or use an appropriate
Error variant such as Error::EscrowExpired), then continue with
escrow.creator.require_auth() and the existing status check/validate_note_length
calls.
| #[contracttype] | ||
| #[derive(Clone, Debug)] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n contracts/split-escrow/src/types.rs | sed -n '455,500p'Repository: OlufunbiIK/StellarSplit
Length of output: 1669
🏁 Script executed:
rg -n 'fn create_escrow' contracts/split-escrow/src/types.rs -A 30Repository: OlufunbiIK/StellarSplit
Length of output: 978
Remove duplicate #[contracttype] and #[derive(Clone, Debug)] attributes on SplitEscrow, and initialize the note field in create_escrow.
Lines 459–460 have duplicate attributes that will cause conflicting trait implementations and fail compilation. Additionally, the note: String field added at line 493 is not initialized in the create_escrow function (line 629–640), which will cause a compilation error for missing required struct field.
Proposed fixes
Remove duplicate attributes at lines 459–460:
#[contracttype]
#[derive(Clone, Debug)]
-#[contracttype]
-#[derive(Clone, Debug)]
pub struct SplitEscrow {Add note field initialization in create_escrow function:
SplitEscrow {
split_id,
creator: creator.clone(),
description,
total_amount,
amount_collected: 0,
participants,
requester: creator.clone(),
status: EscrowStatus::Active,
deadline,
created_at: env.ledger().timestamp(),
+ note: String::new(),
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/split-escrow/src/types.rs` around lines 459 - 460, Remove the
duplicate attributes by keeping a single #[contracttype] and a single
#[derive(Clone, Debug)] on the SplitEscrow struct declaration (remove the
redundant repeated attributes), and in the create_escrow function initialize the
newly added note: String field when constructing SplitEscrow (set note to the
appropriate value, e.g., an empty string or the passed-in note variable) so the
struct literal in create_escrow includes note and the code compiles.
| /// Optional note/comment attached to the escrow (max 128 bytes) | ||
| pub note: String, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for SplitEscrow struct definition and create_escrow function
rg -n -A 30 'pub struct SplitEscrow' contracts/split-escrow/src/types.rs | head -50Repository: OlufunbiIK/StellarSplit
Length of output: 1055
🏁 Script executed:
#!/bin/bash
# Read the struct definition to see if `note` field exists
sed -n '461,510p' contracts/split-escrow/src/types.rsRepository: OlufunbiIK/StellarSplit
Length of output: 1556
🏁 Script executed:
#!/bin/bash
# Find the create_escrow function
rg -n -A 25 'pub fn create_escrow' contracts/split-escrow/src/types.rsRepository: OlufunbiIK/StellarSplit
Length of output: 832
🏁 Script executed:
#!/bin/bash
# Find all places where SplitEscrow struct literal is constructed
rg -n 'SplitEscrow \{' contracts/split-escrow/src/types.rsRepository: OlufunbiIK/StellarSplit
Length of output: 163
🏁 Script executed:
#!/bin/bash
# Find all Rust files in split-escrow and search for SplitEscrow struct construction
find contracts/split-escrow/src -name "*.rs" -type f | while read file; do
rg -l 'SplitEscrow \{' "$file" 2>/dev/null
doneRepository: OlufunbiIK/StellarSplit
Length of output: 171
🏁 Script executed:
#!/bin/bash
# Find all SplitEscrow struct literal constructions in lib.rs and test.rs
echo "=== lib.rs ===" && rg -n -B2 -A15 'SplitEscrow \{' contracts/split-escrow/src/lib.rs
echo "=== test.rs ===" && rg -n -B2 -A15 'SplitEscrow \{' contracts/split-escrow/src/test.rsRepository: OlufunbiIK/StellarSplit
Length of output: 4301
Initialize the note field in all SplitEscrow struct literals.
The SplitEscrow struct now requires a note: String field (line 493), but several construction sites are missing it:
create_escrow()function (types.rs, line 629–640)- Multiple test cases (test.rs): lines 521, 535, 556, 583, 598
The field is already correctly initialized in lib.rs (line 147), but the remaining locations must be updated to compile.
Example fix for types.rs
pub fn create_escrow(
env: &Env,
split_id: String,
@@
) -> SplitEscrow {
SplitEscrow {
split_id,
creator: creator.clone(),
@@
status: EscrowStatus::Active,
deadline,
created_at: env.ledger().timestamp(),
+ note: String::from_str(env, ""),
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/split-escrow/src/types.rs` around lines 492 - 493, Several
SplitEscrow struct literals are missing the new required field note: String;
update every construction site (notably the create_escrow() function and the
test cases in test.rs) to include a note value (e.g., an empty string or
context-appropriate text) when instantiating SplitEscrow, matching how lib.rs
already supplies note; ensure all SplitEscrow {...} literals include note and
re-run compilation/tests.
1 participant
Hello. I saw this post on drip and I completed it. Kindly approve this pr, assign me the project and it's done. Thanks
Summary by CodeRabbit