Skip to content

Conversation

@davidperk
Copy link

Adds the following DoubleEndedQueue functions that return a bool flag instead of reverting:

tryPushBack
tryPopBack
tryPushFront
tryPopFront
tryFront
tryBack
tryAt

These functions eliminate a redundant SLOAD for calling contracts that currently need to check a condition (e.g., whether the deque is full or empty, or whether the index is out of bounds) before invoking a function in order to not revert, all of which themselves check the same condition and revert if the condition fails.

For example, a calling contract currently needs to check empty before invoking front in order to not revert if the deque is empty. But front already invokes empty itself. Now, a calling contract can optimistically invoke tryFront without having to check empty first and gracefully handle the error if the deque is empty.

  • Tests
  • Documentation
  • Changeset entry (run npx changeset add)

@davidperk davidperk requested a review from a team as a code owner October 26, 2025 17:05
@changeset-bot
Copy link

changeset-bot bot commented Oct 26, 2025

🦋 Changeset detected

Latest commit: 293a992

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
openzeppelin-solidity Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@coderabbitai
Copy link

coderabbitai bot commented Oct 26, 2025

Walkthrough

This pull request introduces non-reverting try-prefixed variants to the DoubleEndedQueue contract. Internal functions (tryPushBack, tryPopBack, tryPushFront, tryPopFront, tryFront, tryBack, tryAt) replace direct reverts with false or zero returns on failure. Public methods (pushBack, pushFront, popBack, popFront, front, back, at) now delegate to these try variants and panic with specific error codes when operations fail. The public API signatures remain unchanged. Complementary test coverage validates try function behavior in empty and non-empty queue states, including event emissions and boundary conditions. A changeset documents the minor version bump for the OpenZeppelin Solidity dependency.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The PR title "Add try DoubleEndedQueue functions" directly and accurately describes the main change in the changeset. The title is concise, specific, and clearly communicates that new try* variant functions are being added to the DoubleEndedQueue contract. A teammate scanning the repository history would immediately understand that this PR introduces non-reverting function alternatives, which matches the actual changes documented in the raw summary where try-prefixed variants are introduced for mutating and read operations.
Description Check ✅ Passed The PR description is clearly related to the changeset and provides relevant context about the changes. It lists all seven try* functions being added (tryPushBack, tryPopBack, tryPushFront, tryPopFront, tryFront, tryBack, tryAt), explains the rationale for their introduction (eliminating redundant SLOADs), provides a concrete example of how they improve the developer experience, and includes a completed checklist showing that tests, documentation, and changeset entries were all addressed. This description aligns well with the actual implementation summarized in the raw summary.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1cf1377 and 0ab7f46.

📒 Files selected for processing (3)
  • .changeset/flat-ideas-count.md (1 hunks)
  • contracts/utils/structs/DoubleEndedQueue.sol (7 hunks)
  • test/utils/structs/DoubleEndedQueue.test.js (3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
  • GitHub Check: Redirect rules - solidity-contracts
  • GitHub Check: Header rules - solidity-contracts
  • GitHub Check: halmos
  • GitHub Check: coverage
  • GitHub Check: tests-foundry
  • GitHub Check: tests-upgradeable
  • GitHub Check: slither
  • GitHub Check: tests
  • GitHub Check: Pages changed - solidity-contracts
🔇 Additional comments (14)
.changeset/flat-ideas-count.md (1)

1-5: LGTM! Changeset properly documents the feature addition.

The minor version bump is appropriate for adding new non-reverting try* functions, which is a backward-compatible API enhancement.

test/utils/structs/DoubleEndedQueue.test.js (6)

40-52: LGTM! Comprehensive validation of try getters on empty queue.

The tests properly verify that tryFront, tryBack, and tryAt return (false, ZeroHash) when the queue is empty, ensuring the non-reverting behavior works as intended.


54-57: LGTM! Event emissions properly validated for try pops on empty queue.

The tests correctly verify that tryPopFront and tryPopBack emit events with (false, ZeroHash) when attempting to pop from an empty queue.


59-64: LGTM! Try push operations validated with content verification.

The tests properly verify that tryPushFront and tryPushBack return true on success and correctly update the queue content, maintaining proper ordering.


89-101: LGTM! Try getters properly validated on non-empty queue.

The tests correctly verify that tryFront, tryBack, and tryAt return (true, value) with the expected values when the queue contains elements.


103-107: LGTM! Out-of-bounds behavior properly validated.

The test correctly verifies that tryAt returns (false, ZeroHash) when accessing an index beyond the queue length, demonstrating proper bounds checking without reverting.


140-152: LGTM! Try pop operations thoroughly validated.

The tests properly verify that tryPopFront and tryPopBack return (true, value) with the correct values on non-empty queues, emit appropriate events, and maintain content integrity after operations.

contracts/utils/structs/DoubleEndedQueue.sol (7)

40-58: LGTM! pushBack refactored to use tryPushBack with proper full-queue detection.

The implementation correctly:

  • Detects full queue condition via backIndex + 1 == begin (accounting for uint128 wraparound)
  • Returns false on full, avoiding revert in the try* variant
  • Maintains backward compatibility by panicking with RESOURCE_ERROR in the public wrapper
  • Optimizes gas by eliminating redundant checks when callers use tryPushBack directly

65-86: LGTM! popBack refactored to use tryPopBack with proper empty-queue detection.

The implementation correctly:

  • Detects empty queue via backIndex == begin
  • Returns (false, bytes32(0)) when empty
  • Properly cleans up storage with delete deque._data[backIndex]
  • Maintains backward compatibility with EMPTY_ARRAY_POP panic in the public wrapper

93-111: LGTM! pushFront refactored to use tryPushFront with correct wraparound handling.

The implementation correctly:

  • Calculates frontIndex = deque._begin - 1 (intentionally wrapping uint128 when begin is 0)
  • Detects full queue via frontIndex == end
  • Maintains consistent error handling with RESOURCE_ERROR panic

118-139: LGTM! popFront refactored to use tryPopFront with proper state management.

The implementation correctly:

  • Detects empty queue via frontIndex == end
  • Returns (false, bytes32(0)) when empty
  • Cleans up storage and updates begin pointer appropriately
  • Maintains backward compatibility with EMPTY_ARRAY_POP panic

146-160: LGTM! front refactored to use tryFront with appropriate panic code.

The implementation correctly:

  • Leverages the existing empty() helper for clarity
  • Returns (false, bytes32(0)) when queue is empty
  • Uses ARRAY_OUT_OF_BOUNDS panic code, consistent with array access semantics

167-183: LGTM! back refactored to use tryBack with safe unchecked arithmetic.

The implementation correctly:

  • Uses empty() check before accessing data[end - 1]
  • Safely uses unchecked block since empty check guarantees end > begin
  • Maintains consistent error semantics with ARRAY_OUT_OF_BOUNDS panic

191-209: LGTM! at refactored to use tryAt with proper bounds checking.

The implementation correctly:

  • Validates index >= length() to prevent out-of-bounds access
  • Safely downcasts index to uint128 in unchecked block (guaranteed by length check, as noted in comment)
  • Returns (false, bytes32(0)) for invalid indices without reverting
  • Maintains consistent panic code ARRAY_OUT_OF_BOUNDS in public wrapper

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.

ernestognw
ernestognw previously approved these changes Oct 27, 2025
Copy link
Member

@ernestognw ernestognw left a comment

Choose a reason for hiding this comment

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

Hi @davidperk, thanks for the PR.

I personally like the idea so I'm approving, but we'll need another review from someone else in the team to merge.

A couple of comments:

  • Maybe it makes sense to keep the original functions implemented as they currently are. The new versions depending on the try* ones might be a couple gas units more expensive to push the success and value into the stack and then reusing it. Not a big deal imo
  • Technically, users can check whether the function they're using will revert beforehand by checking _begin and _end. We don't recommend accessing these values directly so maybe checking empty(), front() and back() would do the job. I still think this PR is better.

@davidperk
Copy link
Author

davidperk commented Oct 28, 2025

Maybe it makes sense to keep the original functions implemented as they currently are. The new versions depending on the try* ones might be a couple gas units more expensive to push the success and value into the stack and then reusing it. Not a big deal imo

I gas-profiled some of the original function implementations vs. the PR versions via npm run gas-report -- test/utils/structs/DoubleEndedQueue.test.js:

Function Original (Avg.) PR (Avg.)
popBack 28,189 28,280 (+91)
popFront 28,202 28,287 (+85)
pushBack 57,342 57,415 (+73)
pushFront 49,130 49,212 (+82)

I think it makes sense to keep the original function implementations at the expense of DRY, so I've just restored them in f9b63dc.

Technically, users can check whether the function they're using will revert beforehand by checking _begin and _end. We don't recommend accessing these values directly so maybe checking empty(), front() and back() would do the job. I still think this PR is better.

Yes, technically, users can manually check empty() before calling front() or back(), but that would mean empty() gets called twice, whereas calling tryFront() or tryBack() means empty() only gets called once.

Copy link
Member

@ernestognw ernestognw left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to prepare the benchmarks.
LGTM

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants