feat(salary): implement batch update for employee salary commitments - #40
feat(salary): implement batch update for employee salary commitments#40sudo-robi wants to merge 5 commits into
Conversation
- Added batch_update_commitments entrypoint iterating over a vector of (Address, BytesN<32>) to avoid multiple transaction calls. - Updated Cargo.toml to add 'testutils' feature. - Added comprehensive unit tests test_batch_update_commitments and test_batch_update_fails_if_missing.
|
Hey @sudo-robi! 👋 It looks like this PR isn't linked to any issue. If this PR is for one of the issues assigned to you as part of a Wave, please link it to ensure your contribution is tracked properly. You can do this by adding a keyword to the PR description (e.g.,
|
There was a problem hiding this comment.
Pull request overview
Adds a new batch entrypoint to the salary_commitment Soroban contract to update multiple employees’ commitment hashes in a single transaction, reducing per-employee transaction overhead.
Changes:
- Introduces
batch_update_commitmentsthat iterates through(Address, BytesN<32>)updates and persists new commitment data while bumpingversion. - Adds unit tests for the batch happy path and the missing-commitment failure case.
- Exposes a crate-level
testutilsfeature to enablesoroban-sdk/testutilsfor upstream/workspace testing.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
contracts/salary_commitment/src/lib.rs |
Adds the batch update entrypoint and corresponding unit tests. |
contracts/salary_commitment/Cargo.toml |
Exposes a testutils feature for consumers/tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Batch update existing salary commitments | ||
| pub fn batch_update_commitments( | ||
| env: Env, | ||
| updates: soroban_sdk::Vec<(Address, BytesN<32>)>, | ||
| ) { |
There was a problem hiding this comment.
batch_update_commitments performs privileged state changes but does not enforce any authorization. The PR description says an admin should be able to update multiple employees in one transaction, but as written any caller can update any employee’s commitment hashes. Add an auth check (e.g., require auth from an admin/owner address stored in contract state, or otherwise gate updates) and cover the unauthorized case in tests.
| existing.commitment = new_commitment; | ||
| existing.updated_at = timestamp; | ||
| existing.version += 1; | ||
|
|
There was a problem hiding this comment.
existing.version += 1 can overflow (and this workspace enables overflow-checks = true in release), which would abort the transaction once the counter reaches u32::MAX. Use checked arithmetic and a clear failure mode (or widen the type) so versioning can’t unexpectedly trap.
| #[test] | ||
| #[should_panic(expected = "Commitment not found")] | ||
| fn test_batch_update_fails_if_missing() { | ||
| let env = Env::default(); | ||
| let contract_id = env.register_contract(None, SalaryCommitmentContract); | ||
| let client = SalaryCommitmentContractClient::new(&env, &contract_id); | ||
|
|
||
| let emp_valid = Address::generate(&env); | ||
| let emp_missing = Address::generate(&env); | ||
|
|
||
| client.store_commitment(&emp_valid, &BytesN::from_array(&env, &[1u8; 32])); | ||
|
|
||
| let updates = soroban_sdk::Vec::from_array( | ||
| &env, | ||
| [ | ||
| (emp_valid.clone(), BytesN::from_array(&env, &[10u8; 32])), | ||
| (emp_missing.clone(), BytesN::from_array(&env, &[20u8; 32])), | ||
| ] | ||
| ); | ||
|
|
||
| client.batch_update_commitments(&updates); | ||
| } |
There was a problem hiding this comment.
test_batch_update_fails_if_missing asserts the panic message, but it doesn’t verify the key behavioral guarantee described in the PR (“entire batch transaction … reverts”). Add an assertion after the failed call (e.g., re-read emp_valid’s commitment) to ensure no partial update is persisted when one element in the batch is missing.
romeoscript
left a comment
romeoscript
left a comment
There was a problem hiding this comment.
The CI workflow is currently failing due to cargo fmt. Please run cargo fmt locally to format the code correctly and push the changes:
cargo fmt
|
@sudo-robi this PR currently has merge conflicts. Please resolve the conflicts before it can be merged automatically. |
feat(salary): implement batch update of commitments
Summary
This PR introduces the
batch_update_commitmentsentrypoint to thesalary_commitmentcontract. For companies with numerous employees, issuing individual transactions to update each salary commitment was inefficient. This allows an admin to update multiple employee salary commitments in a single transaction by passing a batch array, significantly reducing network transaction overhead and fee consumption.Closes #
Type of Change
Description of Changes
salary_commitment/src/lib.rs: Added thebatch_update_commitmentsentrypoint which iterates over a vector of (Address, BytesN<32>).DataKey::Commitment(employee)storage slots. For each valid update, it saves the new hash, assigns the current ledger timestamp toupdated_at, and increments the commitmentversion."Commitment not found".testutilsfeature natively to allow upstream workspace testing.test_batch_update_commitments) and the failure bounds (test_batch_update_fails_if_missing).Checklist
General
cargo fmt --checkpasses)cargo clippypasses)cargo test)Smart Contracts (if applicable)
require_auth,require_auth_for_args) are correct and testedZK Circuits (if applicable)
circom <circuit>.circom --r1cs --wasm --symSecurity
cargo audit)Testing Evidence