[N-02 and N-04] Collections audit

[ericnordelo]

Summary by CodeRabbit

• Documentation

  • Clarified SortedMap/SortedSet draining requirements, including how to handle non-droppable keys/values.
  • Expanded pagination and keyset-cursor semantics across mutations, and clarified empty-collection behavior.
  • Improved error-name qualification and tightened security guidance and expected-failure notes.

• Tests

  • Updated expected-failure location pinning and clarified entry/key-return behavior and scenario comments.
  • Added a test helper to construct SortedSet<DropKey> from a vector<u64> of ids.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.66%. Comparing base (15eb9ac) to head (f025249).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #492      +/-   ##
==========================================
- Coverage   96.68%   96.66%   -0.03%     
==========================================
  Files          39       39              
  Lines        3895     3895              
  Branches      875      875              
==========================================
- Hits         3766     3765       -1     
  Misses         74       74              
- Partials       55       56       +1     

Flag	Coverage Δ
contracts/access	65.46% <ø> (ø)
contracts/allowance	52.40% <ø> (ø)
contracts/finance	26.66% <ø> (ø)
contracts/sale	48.59% <ø> (ø)
contracts/timelock	54.32% <ø> (ø)
contracts/utils	44.09% <ø> (ø)
math/core	86.89% <ø> (-0.08%)	⬇️
math/fixed_point	63.37% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 37 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 58ad48be-8e8e-4b73-b1d3-1b1303ab9400

📥 Commits

Reviewing files that changed from the base of the PR and between f88735c and f025249.

📒 Files selected for processing (2)

• collections/README.md
• collections/tests/sorted_set/test_util.move

📝 Walkthrough

Walkthrough

Documentation and test contracts for SortedMap and SortedSet were clarified, including draining, abort attribution, comparator behavior, keyset pagination, and ability requirements. A SortedSet<DropKey> helper and expanded constructor assertions were added; runtime logic and public collection signatures remain unchanged.

Changes

SortedMap contracts and examples

Layer / File(s)	Summary
SortedMap contracts and examples collections/README.md, collections/sources/sorted_map.move, collections/examples/sorted_map/*	Clarifies draining, vector bounds errors, returned entries, pagination semantics, and qualified abort names. The README PriceBook example adds object identity and zero-size validation.

SortedSet contracts and examples

Layer / File(s)	Summary
SortedSet contracts and examples collections/sources/sorted_set.move, collections/README.md, collections/examples/sorted_set/*	Clarifies comparator guarantees, remove! returns, cursor pagination, delegated abort locations, watchlist shapes, and empty-queue behavior.

Test contract alignment

Layer / File(s)	Summary
SortedMap and SortedSet test coverage collections/tests/sorted_map/*, collections/tests/sorted_set/*	Updates expected-failure location metadata and test documentation, adds from_sorted_dk, and expands DropKey constructor and deduplication assertions.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

• OpenZeppelin/contracts-sui#454: Adds the related SortedSet test utility extended by this change.

Suggested reviewers: immrsd, 0xneshi, bidzyyys

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	No pull request description was provided, so the required issue reference, change summary, and checklist items are missing.	Add the required PR template sections, including a Resolves # issue reference, a change summary, and the PR checklist items.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is related to the changeset and concisely signals a collections-focused audit, though it is somewhat broad.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/collections-audit-N-02-N-04

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (1)

collections/README.md (1)

67-71: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Keep the price-book constructor composable.

deploy_and_share consumes the new PriceBook inside transfer::share_object and returns nothing, preventing callers from composing setup or choosing the final ownership action in the same PTB. Prefer returning the object and show sharing as an explicit finalizer step.

As per path instructions, integration examples should follow the composability and return-for-chaining guidance.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@collections/README.md` around lines 67 - 71, Update deploy_and_share to
return the newly constructed PriceBook instead of consuming it with
transfer::share_object; make it a composable constructor and add the integration
example’s sharing as a separate explicit finalizer step using the returned
object.

Source: Path instructions

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@collections/README.md`:
- Line 203: The README comparator guidance incorrectly claims is_well_formed_by!
detects all inconsistent or non-strict comparators. Update the paragraph to
state that it checks only adjacent ordering violations, note that non-strict
comparators such as <= can pass when adjacent keys satisfy the relation, and
avoid claiming keys() is universally available since it requires K: copy. Direct
non-strict and coarse comparator failures to comparator-law and cardinality
checks.

In `@collections/sources/sorted_map.move`:
- Around line 674-675: Document caller-supplied comparator aborts in the public
`_by` API contracts: update sorted_map.move lines 674-675 for the removal API
alongside EKeyNotFound, sorted_map.move lines 877-884 for keys_from_by, and
sorted_set.move lines 625-633 for delegated pagination. Include the comparator
abort in each function’s listed aborts while preserving the existing contracts.

In `@collections/tests/sorted_set/test_util.move`:
- Around line 358-361: Expand the documentation for public function
from_sorted_dk to state that ids must be sorted and that duplicate adjacent IDs
are de-duplicated, then add an “#### Aborts” section naming
sorted_set::EKeysNotSorted as the abort for unsorted IDs.

---

Nitpick comments:
In `@collections/README.md`:
- Around line 67-71: Update deploy_and_share to return the newly constructed
PriceBook instead of consuming it with transfer::share_object; make it a
composable constructor and add the integration example’s sharing as a separate
explicit finalizer step using the returned object.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 0b236443-80b2-4f87-8899-382f0dfca58f

📥 Commits

Reviewing files that changed from the base of the PR and between 15eb9ac and 4a17cfc.

📒 Files selected for processing (16)

• collections/README.md
• collections/examples/sorted_map/order_book.move
• collections/examples/sorted_map/prize_vault.move
• collections/examples/sorted_map/tick_registry.move
• collections/examples/sorted_set/tests/unlock_queue_tests.move
• collections/examples/sorted_set/unlock_queue.move
• collections/sources/sorted_map.move
• collections/sources/sorted_set.move
• collections/tests/sorted_map/comparator_tests.move
• collections/tests/sorted_map/conservation_tests.move
• collections/tests/sorted_map/type_tests.move
• collections/tests/sorted_set/abort_tests.move
• collections/tests/sorted_set/comparator_tests.move
• collections/tests/sorted_set/polarity_tests.move
• collections/tests/sorted_set/test_util.move
• collections/tests/sorted_set/type_tests.move

[bidzyyys]

LGTM