Summary
campaign/src/lib.rs::donate reads the donor record from storage twice in immediate succession. The first read is bound to _donor_record and immediately discarded; the second read binds to existing_donor which is the one actually used (and re-fetched into mut donor_record).
Location
campaign/src/lib.rs, around lines 162–170 (inside pub fn donate(...)):
let _donor_record =
get_donor(&env, &donor).unwrap_or(DonorRecord::new_for(donor.clone(), asset.clone()));
// Update donor record
let existing_donor = get_donor(&env, &donor);
let is_new_donor = existing_donor.is_none();
let mut donor_record =
existing_donor.unwrap_or_else(|| DonorRecord::new_for(donor.clone(), asset.clone()));Impact
- Wasted host storage calls: Three separate calls to
get_donor (the third is implicit via unwrap_or_else if the record is missing), each one reading persistent storage and bumping the donor-record TTL via bump_persistent. On a hot donation path this is pure overhead.
- Allocates an unused
DonorRecord: DonorRecord::new_for(donor.clone(), asset.clone()) is constructed for the discarded read — cloning the Address and AssetInfo for nothing.
- Aesthetics: The code visually suggests the contract behaves differently depending on the first read, when in fact only the second and third matter.
The same pattern (read-once-discard, read-again) exists in claim_refund (separate issue, see [follow-up link once created]).
Fix
Delete the leading _donor_record line entirely; the rest of the block already does the right thing:
let existing_donor = get_donor(&env, &donor);
let is_new_donor = existing_donor.is_none();
let mut donor_record =
existing_donor.unwrap_or_else(|| DonorRecord::new_for(donor.clone(), asset.clone()));Severity
[LOW] — dead code, no security or correctness impact. It just inflates the per-donation host-cost and confuses readers about which value is being honoured.
Summary
campaign/src/lib.rs::donatereads the donor record from storage twice in immediate succession. The first read is bound to_donor_recordand immediately discarded; the second read binds toexisting_donorwhich is the one actually used (and re-fetched intomut donor_record).Location
campaign/src/lib.rs, around lines 162–170 (insidepub fn donate(...)):Impact
get_donor(the third is implicit viaunwrap_or_elseif the record is missing), each one reading persistent storage and bumping the donor-record TTL viabump_persistent. On a hot donation path this is pure overhead.DonorRecord:DonorRecord::new_for(donor.clone(), asset.clone())is constructed for the discarded read — cloning theAddressandAssetInfofor nothing.The same pattern (read-once-discard, read-again) exists in
claim_refund(separate issue, see [follow-up link once created]).Fix
Delete the leading
_donor_recordline entirely; the rest of the block already does the right thing:Severity
[LOW]— dead code, no security or correctness impact. It just inflates the per-donation host-cost and confuses readers about which value is being honoured.