Skip to content

fix/batches-result#196

Merged
RanaBug merged 1 commit into
masterfrom
fix/batches-result
Nov 6, 2025
Merged

fix/batches-result#196
RanaBug merged 1 commit into
masterfrom
fix/batches-result

Conversation

@RanaBug
Copy link
Copy Markdown
Collaborator

@RanaBug RanaBug commented Nov 6, 2025

Description

  • Fix result and userOphash for batches

How Has This Been Tested?

  • Unit Tests and Manual Testing

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Summary by CodeRabbit

  • Bug Fixes
    • Fixed batch operation results to correctly return complete transaction data with all required fields and proper nested structures.
    • Improved error handling for non-existent batch requests. The system now explicitly reports failed batch requests with descriptive error messages and clear status indicators, instead of silently returning empty results for better debugging experience.

@RanaBug RanaBug requested a review from IAmKio November 6, 2025 15:51
@RanaBug RanaBug self-assigned this Nov 6, 2025
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Nov 6, 2025

Walkthrough

The PR refactors return payloads in batch estimation and sending methods to return result objects directly instead of merging them with internal kit state, updates corresponding test assertions to reflect the new behavior, and bumps the version to 2.1.3.

Changes

Cohort / File(s) Summary
Version & Documentation
CHANGELOG.md, package.json
Added 2.1.3 patch entry documenting sendBatches() and estimateBatches() now return correct batch results with chainGroups and userOpHash data; version bumped from 2.1.2 to 2.1.3
Test Updates
__tests__/EtherspotTransactionKit.test.ts
Updated batch estimation tests to check nested transactions property and expect explicit error entries for non-existent batches with errorMessage and isEstimatedSuccessfully flags
Core Logic
lib/TransactionKit.ts
Removed spread of instance state (this) from return payloads in estimate() and estimateBatches() paths; now returns only result object instead of merged state

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

  • Consistent pattern of removing this spread operator from return statements
  • Test updates follow straightforward assertion pattern changes
  • Changes maintain control flow; only payload structure differs
  • Version bump and documentation are straightforward

Possibly related PRs

Suggested reviewers

  • IAmKio

Poem

🐰 No more internal state spread across the sky,
Just pure results for batches passing by,
The payload's cleaner, the tests ring true,
Version 2.1.3—a fresher view! ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix/batches-result' directly relates to the main changeset, which fixes batch result handling to return correct batch structure with transactions and error entries.
Description check ✅ Passed The description includes all required template sections: a summary of changes ('Fix result and userOphash for batches'), testing details ('Unit Tests and Manual Testing'), and selected change type (Bug fix checkbox marked).
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/batches-result

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Collaborator

@IAmKio IAmKio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@RanaBug RanaBug merged commit 9274f02 into master Nov 6, 2025
4 of 5 checks passed
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (3)
lib/TransactionKit.ts (1)

4214-4219: sendBatches now returns only result at function end.

Consistent payload and easier typing for consumers; nice cleanup.

For consistency, consider aligning single-tx estimate()/send() to also return plain results (no ...this) in a follow-up patch to avoid differing shapes across APIs.

__tests__/EtherspotTransactionKit.test.ts (2)

964-967: Assert on nested batch transactions length.

Good update matching new result.batches[batchName].transactions shape.

Optionally also assert result.batches['batch1'].chainGroups exists to cover group aggregation.


977-982: Explicit error entry for non-existent batch.

Solid: verifies presence of error metadata and failure flag for missing batch.

Add expect(result.batches['doesnotexist'].transactions).toHaveLength(0) and, if available, expect(result.batches['doesnotexist'].errorType).toBe('VALIDATION_ERROR') for completeness.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 66b5373 and 5e67278.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (4)
  • CHANGELOG.md (1 hunks)
  • __tests__/EtherspotTransactionKit.test.ts (2 hunks)
  • lib/TransactionKit.ts (4 hunks)
  • package.json (1 hunks)
🔇 Additional comments (6)
package.json (1)

4-4: Version bump acknowledged (2.1.3).

Looks good for a patch release tied to batch result shape fixes. Ensure CHANGELOG and tag align on publish.

lib/TransactionKit.ts (3)

2344-2347: Empty-batches early return now returns a plain result object.

Good: prevents leaking instance state in batch API. Matches stated behavior change.


3061-3066: Modular estimateBatches final return simplified to result.

Aligns modular and delegatedEoa branches; clearer contract for callers.


2755-2760: Batch methods correctly return only result; no API surface mixing detected.

Verification confirms estimateBatches and sendBatches methods return only result without spreading this. All 13 matches of { ...result, ...this } found in the codebase occur in non-batch estimate() and send() methods (lines 1099, 1124, 1167, 1458, 1559, 1653, 1679, 1723, 2042, 2231), which are before line 2289 where estimateBatches begins. The batch methods maintain clean API boundaries as intended.

CHANGELOG.md (2)

3-7: Verify release date for version 2.1.3.

The changelog entry is clear and accurately describes the fix to sendBatches() and estimateBatches() return values. However, the date 2025-01-27 matches versions 2.1.2, 2.1.1, and 2.1.0 below it, which is unusual for sequential patch/minor releases. Typically, each release has its own date reflecting when it was published.

Confirm whether version 2.1.3 was released on the same date as the previous versions or if the date should be updated to reflect the actual release date.


3-7: Changelog entry is clear and well-structured.

The entry effectively communicates what was fixed: the return values of sendBatches() and estimateBatches() now correctly include chainGroups and userOpHash data. The description aligns with the PR objectives and maintains consistency with the existing changelog format.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants