feat: add collections (sorted_map, sorted_set, big_sorted_map)#419
feat: add collections (sorted_map, sorted_set, big_sorted_map)#419kosedogus wants to merge 9 commits into
Conversation
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #419 +/- ##
==========================================
+ Coverage 96.26% 96.35% +0.09%
==========================================
Files 29 31 +2
Lines 2968 3014 +46
Branches 732 736 +4
==========================================
+ Hits 2857 2904 +47
Misses 66 66
+ Partials 45 44 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
kosedogus
requested review from
0xNeshi,
bidzyyys,
ericnordelo and
immrsd
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (21)
collections/big_sorted_map/examples/big_sorted_map/clmm_pool.move-68-79 (1)
68-79: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftModel the pool lifecycle as create-then-share.
deploy_and_sharebakes the share step into construction, so callers cannot compose with the ownedPoolbefore it becomes shared. Since this module is an example, that teaches the opposite of the repo’s shared-object pattern. As per path instructions, "shared objects must use the two-step “create then share” pattern" and "prefer functions that return objects over self-transferring them; transferring breaks PTB composition."🤖 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/big_sorted_map/examples/big_sorted_map/clmm_pool.move` around lines 68 - 79, `deploy_and_share` currently mixes object creation and sharing, which prevents callers from composing with the owned `Pool` before it becomes shared. Refactor the example to follow the create-then-share pattern by separating `Pool` construction from the `transfer::share_object` step, so the function first returns or exposes the owned `Pool` (or an equivalent create helper) and only a distinct follow-up action performs sharing. Keep the fix centered on `deploy_and_share` and the `Pool` construction flow.Source: Path instructions
collections/big_sorted_map/examples/big_sorted_map/clmm_pool.move-82-87 (1)
82-87: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon't clear
crossedon overwrite.Replacing an existing tick always writes
crossed: false. If that tick was already counted intoactive_liquidity, a latercross_up_fromwill add it again and drift the pool state. Either make this API initialization-only or add a dedicated update path that reconcilesactive_liquiditywhen a crossed tick changes.🤖 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/big_sorted_map/examples/big_sorted_map/clmm_pool.move` around lines 82 - 87, The set_tick helper in clmm_pool.move is overwriting existing Tick values by always resetting crossed to false, which can desynchronize active_liquidity for already-crossed ticks. Update the set_tick path (or split it into initialization-only and update APIs) so that overwrite behavior preserves or reconciles the existing crossed state instead of clearing it. Use the set_tick function and the Tick struct as the key symbols when adjusting the insert logic, and ensure any update path keeps active_liquidity consistent with cross_up_from.collections/big_sorted_map/examples/big_sorted_map/order_book.move-89-120 (1)
89-120: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftSplit book construction from sharing.
Both
deploy_and_shareandmigrate_and_shareself-share theOrderBook, which removes the owned-object step that the repo’s shared-object examples are supposed to demonstrate. Returning the book and sharing it in a dedicated wrapper keeps this example PTB-friendly and aligned with the architecture contract. As per path instructions, "shared objects must use the two-step “create then share” pattern" and "prefer functions that return objects over self-transferring them; transferring breaks PTB composition."🤖 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/big_sorted_map/examples/big_sorted_map/order_book.move` around lines 89 - 120, Both deploy_and_share and migrate_and_share are self-sharing OrderBook instances, but the example should keep creation separate from sharing. Refactor the OrderBook construction in deploy_and_share and migrate_and_share so they return the created object (or its ID) without calling transfer::share_object, and move the sharing step into a dedicated wrapper/helper that uses the returned value. Keep the relevant symbols OrderBook, deploy_and_share, migrate_and_share, and transfer::share_object in mind while splitting the create-then-share flow.Source: Path instructions
collections/big_sorted_map/sources/big_sorted_map.move-261-265 (1)
261-265: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftNarrow the mutable cursor surface before this package ships.
borrow_node_mutplusnode_leaf_muthand external callers an arbitrary&mut SortedMap<K, V>for a leaf. That lets them callinsert_at,remove_at,append,split_off, etc. without updatingBigSortedMap.length, routing keys, or rebalance state, which can permanently desync persisted tree state.leaf_value_at_mutalready covers the documented DIY-cursor need for in-place value mutation, so this extra public surface is wider than necessary. As per path instructions, "Package boundaries are audit boundaries: adding new packages/modules increases audit surface; ensure any exported/public API is deliberate and stable".Also applies to: 306-310
🤖 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/big_sorted_map/sources/big_sorted_map.move` around lines 261 - 265, The public mutable leaf accessors expose too much of the internal tree and can let callers mutate a leaf SortedMap without keeping BigSortedMap state in sync. Restrict or remove the exported mutable cursor surface by making borrow_node_mut/node_leaf_mut non-public or otherwise internal-only, and keep leaf_value_at_mut as the supported public path for in-place value mutation. Update the relevant BigSortedMap and Node helper APIs so external callers cannot directly obtain &mut SortedMap<K, V> from a leaf.Source: Path instructions
collections/sorted_map/examples/sorted_map/tests/tick_registry_tests.move-112-117 (1)
112-117: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winEnd the expected-failure test at the abort site.
ts::return_shared(reg)andts::end(scenario)are unreachable afterborrow_tickaborts, and the STYLEGUIDE explicitly asks expected-failure tests to stop at the failure boundary with no cleanup. As per coding guidelines,No cleanup in expected-failure tests; abort at the failure boundary.🤖 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/sorted_map/examples/sorted_map/tests/tick_registry_tests.move` around lines 112 - 117, The expected-failure test in tick_registry_tests.move should stop immediately at the aborting call site inside the test body. Remove the unreachable cleanup and scenario ավարտ code after tick_registry::borrow_tick on the inactive tick, and leave the test ending at the failure boundary with no ts::return_shared or ts::end in that path.Sources: Coding guidelines, Path instructions
collections/sorted_map/examples/sorted_map/tests/tick_registry_tests.move-31-33 (1)
31-33: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse
assert_eqinstead ofassert!throughout this walkthrough.The new test suites are supposed to use
std::unit_test::assert_eq, including these boolean result checks. As per coding guidelines,Use std::unit_test::assert_eq for assertions (avoid abort-code arithmetic in assert!).Also applies to: 41-41, 56-58, 67-68, 73-76, 84-85
🤖 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/sorted_map/examples/sorted_map/tests/tick_registry_tests.move` around lines 31 - 33, The walkthrough tests in tick_registry_tests.move still use assert! for boolean checks; update these assertions to use std::unit_test::assert_eq consistently throughout the test module. Replace the boolean result assertions around tick_registry::add_tick and the other listed checks with assert_eq calls, keeping the same expected values so the test intent remains unchanged.Sources: Coding guidelines, Path instructions
collections/sorted_map/examples/sorted_map/prize_vault.move-32-122 (1)
32-122: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAlign this module with the required STYLEGUIDE structure and docs.
This file is missing the canonical
// === ... ===section layout, the#[test_only]helper is not under the exact// === Test-Only Helpers ===section, and the public APIs do not use the required#### Parameters/#### Returns/#### Abortsdoc sections. As per coding guidelines,STYLEGUIDE.mdis authoritative here:Follow exact file section ordering using // === <Name> === delimitersandinclude #### Parameters, #### Returns, and #### Aborts whenever relevant.🤖 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/sorted_map/examples/sorted_map/prize_vault.move` around lines 32 - 122, The module needs to be reformatted to match the required STYLEGUIDE section layout and doc structure. Add the canonical // === <Section> === delimiters in the expected order, move the #[test_only] helper into the exact // === Test-Only Helpers === section, and update each public API in prize_vault to use the required doc subsections. Make sure create, fund, pay_next, pay_rank, unclaimed, and close all include the appropriate #### Parameters, #### Returns, and #### Aborts sections where relevant, while keeping the existing identifiers and behavior unchanged.Sources: Coding guidelines, Path instructions, Learnings
collections/sorted_map/examples/sorted_map/order_book.move-37-76 (1)
37-76: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftThis shared order book leaves a vector-backed map unbounded.
SortedMapis the small, vector-backed ordered collection, butOrderBookis a long-lived shared object with no cap on ask/bid levels. That conflicts with the repo’s bounded-state rule and will eventually turn into object-size / gas problems for exactly this kind of workload. Please either enforce a hard bound (documented and checked inplace_ask/place_bid) or move this example tobig_sorted_map. As per path instructions,avoid storing unbounded vectors inside long-lived objectsand this is called out as especially relevant to ordered collections.🤖 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/sorted_map/examples/sorted_map/order_book.move` around lines 37 - 76, The shared OrderBook example stores unbounded SortedMap-backed state in a long-lived shared object, which violates the bounded-state guidance. Update OrderBook and the place_ask/place_bid flows to enforce a hard maximum number of price levels with a checked failure path, or refactor this example to use big_sorted_map instead of SortedMap. Make sure the fix is anchored around OrderBook, deploy_and_share, place_ask, and place_bid so the object cannot grow without bound.Sources: Path instructions, Learnings
collections/sorted_map/examples/sorted_map/tests/order_book_tests.move-113-118 (1)
113-118: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winEnd the expected-failure test at the abort site.
ts::return_shared(book)andts::end(scenario)are unreachable afterorder_book::ask_size_ataborts, and the STYLEGUIDE explicitly asks these tests to stop at the failure boundary with no cleanup. As per coding guidelines,No cleanup in expected-failure tests; abort at the failure boundary.🤖 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/sorted_map/examples/sorted_map/tests/order_book_tests.move` around lines 113 - 118, The expected-failure test should stop immediately at the abort boundary, since `order_book::ask_size_at` is the intended failure point. Remove the unreachable cleanup and scenario finalization after the abort in this test, including `ts::return_shared(book)` and the trailing `ts::end(scenario)` block. Keep the test focused on `order_book::ask_size_at` and let it terminate at the failure site with no cleanup.Sources: Coding guidelines, Path instructions
collections/sorted_map/examples/sorted_map/tests/prize_vault_tests.move-133-139 (1)
133-139: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winStop both expected-failure tests at the aborting call.
These scenarios keep
ts::end(...), andpay_next_on_empty_vault_abortsalso keeps unreachable cleanup after the abort. The STYLEGUIDE calls out the opposite pattern for expected-failure tests. As per coding guidelines,No cleanup in expected-failure tests; abort at the failure boundary.Also applies to: 163-171
🤖 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/sorted_map/examples/sorted_map/tests/prize_vault_tests.move` around lines 133 - 139, The expected-failure tests in prize_vault_tests.move should stop at the aborting call instead of continuing with cleanup. Update the test cases that use prize_vault::close and pay_next_on_empty_vault_aborts so they end immediately after the failure boundary, and remove any unreachable cleanup or ts::end(...) that follows the abort. Keep the fix localized to the affected test functions in prize_vault_tests and preserve the intended failure assertion behavior.Sources: Coding guidelines, Path instructions
collections/sorted_map/examples/sorted_map/order_book.move-47-55 (1)
47-55: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftSplit creation from sharing in the example lifecycle.
This constructor bakes
transfer::share_objectinto the API and only returns theID, which is the opposite of the repo’s documented shared-object pattern and hurts PTB composability. Please expose a create step that returns the owned object, then a separate share step. As per path instructions, shared objects must use the two-step “create then share” pattern and should prefer returning objects over self-transferring them.🤖 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/sorted_map/examples/sorted_map/order_book.move` around lines 47 - 55, The deploy_and_share example currently mixes object creation and sharing, and returns only an ID from the constructor. Update the OrderBook lifecycle to follow the shared-object pattern by splitting the current deploy_and_share flow into a create function that returns the owned OrderBook and a separate share function that calls transfer::share_object; keep the ID retrieval in the create path if needed, but avoid self-sharing inside the constructor so the example stays PTB-composable.Sources: Path instructions, Learnings
collections/sorted_map/examples/sorted_map/tests/order_book_tests.move-64-65 (1)
64-65: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse
assert_eqfor the boolean assertions here.The new test suites are supposed to use
std::unit_test::assert_eq, including the well-formedness checks in this scenario. As per coding guidelines,Use std::unit_test::assert_eq for assertions (avoid abort-code arithmetic in assert!).🤖 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/sorted_map/examples/sorted_map/tests/order_book_tests.move` around lines 64 - 65, The boolean checks in the order book test should use std::unit_test::assert_eq instead of assert! for consistency with the new test suite guidelines. Update the well-formedness assertions in order_book_tests to compare each expression against true using assert_eq, keeping the existing calls to order_book::bids_well_formed and order_book::asks_ref with sorted_map::is_well_formed! unchanged. This should be applied wherever these boolean assertions appear in the test case.Sources: Coding guidelines, Path instructions
collections/sorted_map/examples/sorted_map/tick_registry.move-24-113 (1)
24-113: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAlign this module with the required STYLEGUIDE structure and docs.
This file is missing the canonical
// === ... ===section layout, the#[test_only]helper is not under the exact// === Test-Only Helpers ===section, and several public APIs are missing the required#### Parameters/#### Returns/#### Abortsdoc sections. As per coding guidelines,STYLEGUIDE.mdis authoritative here:Follow exact file section ordering using // === <Name> === delimitersandinclude #### Parameters, #### Returns, and #### Aborts whenever relevant.🤖 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/sorted_map/examples/sorted_map/tick_registry.move` around lines 24 - 113, The module in tick_registry.move needs to be reorganized to match STYLEGUIDE sectioning and doc requirements. Add the canonical // === ... === delimiters in the expected order, move the #[test_only] ticks_well_formed helper under the exact // === Test-Only Helpers === section, and update each public API in TickRegistry to include the required doc subsections. Make sure functions like deploy_and_share, add_tick, remove_tick, contains_tick, borrow_tick, accrue_fees, min_tick, max_tick, tick_above, tick_below, ceiling_tick, floor_tick, length, and is_empty each have the appropriate #### Parameters / #### Returns / #### Aborts blocks where relevant.Sources: Coding guidelines, Path instructions, Learnings
collections/sorted_map/examples/sorted_map/tick_registry.move-35-58 (1)
35-58: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftThis shared tick registry leaves a vector-backed map unbounded.
A CLMM tick registry is exactly the kind of long-lived ordered state the architecture warns about. Backing it with
SortedMapand exposing unconstrainedadd_tickwrites means the object can grow until it hits the vector/object-size limits the repo explicitly wants to avoid. Please cap the active-tick count or switch this example tobig_sorted_map. As per path instructions,avoid storing unbounded vectors inside long-lived objectsand this is especially relevant to ordered collections.🤖 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/sorted_map/examples/sorted_map/tick_registry.move` around lines 35 - 58, The shared TickRegistry example currently uses a vector-backed SortedMap inside a long-lived shared object, which can grow without bound. Update TickRegistry/deploy_and_share/add_tick/remove_tick so the registry either enforces a hard maximum number of active ticks before inserting or is rewritten to use big_sorted_map for ordered storage. Make the fix in the TickRegistry struct and its write path, and ensure add_tick rejects growth past the cap rather than allowing unbounded inserts.Sources: Path instructions, Learnings
collections/sorted_map/examples/sorted_map/tick_registry.move-42-46 (1)
42-46: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftSplit creation from sharing in the example lifecycle.
This API shares the object during construction and only returns the
ID, which does not match the repo’s documented two-step shared-object pattern or PTB-friendly guidance. Please expose a create step that returns the owned object and a separate share step. As per path instructions, shared objects must use the two-step “create then share” pattern and should prefer returning objects over self-transferring them.🤖 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/sorted_map/examples/sorted_map/tick_registry.move` around lines 42 - 46, The TickRegistry example currently combines creation and sharing in deploy_and_share, which should be split into the repo’s two-step shared-object pattern. Refactor the lifecycle into a create function that constructs and returns the owned TickRegistry object, and a separate share function that takes that object and calls transfer::share_object; keep object::id handling in the create step so the example is PTB-friendly and prefers returning the object over self-transferring it.Sources: Path instructions, Learnings
collections/sorted_map/examples/sorted_map/order_book.move-28-129 (1)
28-129: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAlign this module with the required STYLEGUIDE structure and docs.
This file is missing the canonical
// === ... ===section layout, the#[test_only]helpers are not under the exact// === Test-Only Helpers ===heading, and the public APIs do not use the required#### Parameters/#### Returns/#### Abortsdoc sections. As per coding guidelines,STYLEGUIDE.mdis authoritative here:Follow exact file section ordering using // === <Name> === delimitersandinclude #### Parameters, #### Returns, and #### Aborts whenever relevant.🤖 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/sorted_map/examples/sorted_map/order_book.move` around lines 28 - 129, The module layout and doc comments do not match the required STYLEGUIDE structure. Reorganize the `order_book` module so its sections use the exact `// === <Name> ===` delimiters in the canonical order, and rename the test section to `// === Test-Only Helpers ===` above `bids_well_formed` and `asks_ref`. Update each public function in `deploy_and_share`, `place_ask`, `place_bid`, `best_ask`, `best_bid`, `ask_levels`, `ask_size_at`, and `fill_best_ask` to use the required doc sections with `#### Parameters`, `#### Returns`, and `#### Aborts` where applicable.Sources: Coding guidelines, Path instructions, Learnings
collections/sorted_map/examples/sorted_map/prize_vault.move-62-67 (1)
62-67: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftSplit vault creation from sharing.
Like the other examples, this API shares the object inside the constructor and only returns its
ID, which does not match the repo’s documented shared-object lifecycle or PTB-friendly style. Please expose separate create/share steps instead. As per path instructions, shared objects must use the two-step “create then share” pattern and should prefer returning objects over self-transferring them.🤖 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/sorted_map/examples/sorted_map/prize_vault.move` around lines 62 - 67, The create API for PrizeVault currently shares the object inside create and only returns its ID, which breaks the repo’s preferred two-step shared-object lifecycle. Update PrizeVault::create to only construct and return the newly created PrizeVault together with OrganizerCap, and add a separate share function (or equivalent explicit share step) that takes the vault object and performs transfer::share_object. Keep the PTB-friendly flow by returning the object from create rather than self-transferring it.Sources: Path instructions, Learnings
collections/sorted_map/examples/sorted_map/prize_vault.move-80-84 (1)
80-84: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject rank
0before funding the vault.The module documents
1 = champion, butfundcurrently accepts0. Because payouts are ascending, a funded rank0would be paid before the champion and silently break the ordering invariant this example is trying to teach.Suggested fix
#[error(code = 2)] const ERankAlreadyFunded: vector<u8> = "Rank already funded"; +#[error(code = 3)] +const EInvalidRank: vector<u8> = "Rank must be >= 1"; @@ public fun fund(vault: &mut PrizeVault, cap: &OrganizerCap, rank: u64, coin: Coin<SUI>) { assert_cap(vault, cap); + assert!(rank > 0, EInvalidRank); assert!(!sorted_map::contains!(&vault.prizes, &rank), ERankAlreadyFunded); sorted_map::insert!(&mut vault.prizes, rank, coin).destroy_none(); }🤖 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/sorted_map/examples/sorted_map/prize_vault.move` around lines 80 - 84, The fund function in PrizeVault should reject rank 0 before inserting into sorted_map, since the module’s 1-based ranking model starts at champion and a zero rank would break the intended payout ordering. Update fund to validate the rank argument alongside the existing assert_cap and duplicate-rank check, using the existing PrizeVault and fund symbols to locate the change, and fail with an appropriate error when rank is zero.collections/sorted_set/examples/sorted_set/unlock_queue.move-46-50 (1)
46-50: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winExpose a two-step shared-object lifecycle here.
deploy_and_sharecreates and shares the queue in one call, but the repo architecture says shared objects should model a separate create step and share step. Keeping the object-returning constructor as the primary API is also the PTB-friendly shape the repo asks examples to teach.As per path instructions, "shared objects must use the two-step “create then share” pattern" and "prefer functions that return objects over self-transferring them." Based on learnings, "Follow
ARCHITECTURE.mdfor the library’s design principles, including owned vs. shared objects [and] PTB composability."🤖 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/sorted_set/examples/sorted_set/unlock_queue.move` around lines 46 - 50, `deploy_and_share` currently creates and shares `UnlockQueue` in one step, which conflicts with the repo’s shared-object pattern. Split this into a constructor that returns the owned `UnlockQueue` (using `object::new` and `sorted_set::from_keys!`) and a separate share function that calls `transfer::share_object` on that value. Keep the object-returning constructor as the primary API and update any example usage to model the two-step create-then-share flow.Sources: Path instructions, Learnings
collections/sorted_set/examples/sorted_set/validator_set.move-73-77 (1)
73-77: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winThis shared example should use separate create and share steps.
deploy_and_sharebakes creation and sharing into one API, which is the opposite of the repository’s required shared-object lifecycle. A constructor that returns the owned object, plus a separate share helper, would match the documented pattern and keep the example PTB-friendly.As per path instructions, "shared objects must use the two-step “create then share” pattern" and "prefer functions that return objects over self-transferring them." Based on learnings, "Follow
ARCHITECTURE.mdfor the library’s design principles, including owned vs. shared objects [and] PTB composability."🤖 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/sorted_set/examples/sorted_set/validator_set.move` around lines 73 - 77, The `deploy_and_share` example mixes object construction and sharing in one step, which breaks the required shared-object lifecycle. Refactor `deploy_and_share` in `validator_set.move` to follow the repository pattern used for shared objects: add a constructor that returns the owned `ValidatorSet` (similar to the current `ValidatorSet { ... }` creation), then add a separate share helper that calls `transfer::share_object` on that value. Keep the example PTB-friendly by returning the object from the create path and only sharing in the dedicated helper.Sources: Path instructions, Learnings
collections/sorted_set/examples/sorted_set/allowlist.move-69-84 (1)
69-84: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winCap this allowlist or switch the example to the large-workload collection.
Allowlistis a long-lived object backed bySortedSet, but bothcreate(initial)andapprove()allow the embedded set to grow without any bound. That teaches an object shape the repo explicitly tries to avoid for vector-backed collections, because it will eventually run into object-size / gas limits.As per path instructions, "avoid ever-growing vectors in long-lived objects; vectors/Vec* only for bounded ≤1000 items" and "Bounded state: respect the repo’s vector-in-object constraint." Based on learnings, "Follow
ARCHITECTURE.mdfor the library’s design principles ... bounded state."🤖 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/sorted_set/examples/sorted_set/allowlist.move` around lines 69 - 84, The Allowlist example currently models unbounded growth in a long-lived object, which violates the repo’s bounded-state guidance for vector-backed collections. Update the example by either adding an explicit cap/guard to Allowlist creation and approve so members cannot grow past the allowed bound, or move this sample to the large-workload collection if it is meant to be unbounded. Focus the fix around Allowlist, create, and approve so the embedded SortedSet stays within the repository’s object-size and gas constraints.Sources: Path instructions, Learnings
🟡 Minor comments (8)
collections/sorted_map/README.md-108-111 (1)
108-111: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe abort contract is overstated here.
insert_at,remove_at,value_at,value_at_mut, andkey_atare still public and still abort on bad indices instd::vector;collections/sorted_map/sources/sorted_map.move:44-51andcollections/sorted_map/tests/comparator_tests.move:114-129already distinguish that from the supported macro API. Please qualify this bullet to the supported surface or call out the foreign OOB aborts explicitly.🤖 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/sorted_map/README.md` around lines 108 - 111, The abort-contract bullet overstates the library’s guarantees by implying only the listed map operations can abort. Update the README section in the sorted_map docs to clearly distinguish the supported macro/API surface from the still-public helper functions like insert_at, remove_at, value_at, value_at_mut, and key_at, which can still abort on out-of-bounds vector access. Use the existing sorted_map and big_sorted_map terminology to qualify that these foreign OOB aborts are outside the supported total API, and keep the unsupported-internals warning focused on those symbols.collections/sorted_map/README.md-66-69 (1)
66-69: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse the repo’s two-step shared-object lifecycle in the example.
deploy_and_shareteaches create-and-share in one step, but the architecture guidance for these collections packages requires the shared-object “create then share” pattern and prefers return-based flows for PTB composition. Please split this into a constructor that returnsPriceBookplus a separate share step, or explicitly document why this example is the exception.Suggested doc-snippet rewrite
-public fun deploy_and_share(ctx: &mut TxContext) { - let book = PriceBook { id: object::new(ctx), levels: sorted_map::new() }; - transfer::share_object(book); +public fun new_book(ctx: &mut TxContext): PriceBook { + PriceBook { id: object::new(ctx), levels: sorted_map::new() } +} + +public fun share_book(book: PriceBook) { + transfer::share_object(book); }As per path instructions,
shared objects must use the two-step “create then share” patternandprefer functions that return objects over self-transferring them.🤖 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/sorted_map/README.md` around lines 66 - 69, The README example for the PriceBook shared-object flow uses a one-step create-and-share pattern, which conflicts with the repo guidance for shared objects. Update the example around deploy_and_share and PriceBook to show a constructor that returns the object first, then a separate share step using transfer::share_object, or add a brief note explaining why this collection is an exception. Keep the example aligned with the return-based pattern preferred for PTB composition.Source: Path instructions
collections/big_sorted_map/README.md-67-70 (1)
67-70: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse the repo’s two-step share pattern in this example.
deploy_and_sharecreates and shares the wrapper in one step, but the architecture guidance for these new collections packages requires the shared-object lifecycle to be split into create-then-share. As written, the README is teaching the opposite pattern in its primary shared-object example. As per path instructions, "shared objects must use the two-step “create then share” pattern".🤖 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/big_sorted_map/README.md` around lines 67 - 70, The README example for deploy_and_share is using a one-step create-and-share flow, which conflicts with the repo’s required shared-object lifecycle. Update this example to follow the two-step pattern by first creating the Registry wrapper and then sharing it in a separate step, so the BigSortedMap documentation matches the architecture guidance for shared objects.Source: Path instructions
collections/big_sorted_map/sources/big_sorted_map.move-69-72 (1)
69-72: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAlign the top-level section markers with
STYLEGUIDE.md.This module jumps straight from raw
uselines into// === Errors ===and omits several required section delimiters/order slots (Imports,Events,Method Exports,Init,View helpers,Admin Functions,Package Functions). The repo treats that section skeleton as part of the file contract. As per path instructions, "Follow exact file section ordering using// === <Name> ===delimiters (Imports → Errors → Constants → Structs → Events → Method Exports → Init → Public Functions → View helpers → Admin Functions → Package Functions → Private Functions → Test-Only Helpers)".🤖 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/big_sorted_map/sources/big_sorted_map.move` around lines 69 - 72, The module section layout is missing the required top-level delimiters and order from STYLEGUIDE.md. Update the header around the imports in big_sorted_map.move to include the full section skeleton in the prescribed order, keeping the existing Errors section but adding the missing markers such as Imports, Events, Method Exports, Init, View helpers, Admin Functions, and Package Functions where applicable. Use the existing section comments and symbols in the module (for example, the top-level module body and the current // === Errors === marker) to reorganize the file without changing behavior.Source: Path instructions
collections/big_sorted_map/README.md-104-108 (1)
104-108: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the
sorted_map::EBadSplitexception in the abort surface.This section says the abort surface is "all at the library's location", but
collections/big_sorted_map/tests/abort_tests.move:220-275explicitly covers a cross-moduleopenzeppelin_sorted_map::sorted_map::EBadSplitpath whoseexpected_failurelocation isopenzeppelin_sorted_map::sorted_map. Consumers following this README will pin the wrong module location for that case.🤖 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/big_sorted_map/README.md` around lines 104 - 108, The abort-surface summary in the BigSortedMap README is missing the cross-module `openzeppelin_sorted_map::sorted_map::EBadSplit` case, so update the “Seven aborts” list to explicitly call out that exception and its `expected_failure` location. Keep the existing library-local aborts unchanged, but note that this one is reported from `openzeppelin_sorted_map::sorted_map` rather than `openzeppelin_big_sorted_map::big_sorted_map`. Use the `into_sorted_map`/`from_sorted_map` abort-surface wording nearby to place the new exception in the right spot.collections/big_sorted_map/sources/big_sorted_map.move-348-355 (1)
348-355: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake
leaf_next/leaf_prevreject inner nodes.These helpers read raw sibling-link fields without checking
n.is_leaf, but inner-node links are not maintained as part of the public cursor contract. A wrong-node call can therefore return stale ids and send external cursor code down the wrong path instead of failing withEWrongNodeKind.Suggested hardening
public fun leaf_next<K: copy + drop + store, V: store>(n: &Node<K, V>): u64 { + assert!(n.is_leaf, EWrongNodeKind); n.next } public fun leaf_prev<K: copy + drop + store, V: store>(n: &Node<K, V>): u64 { + assert!(n.is_leaf, EWrongNodeKind); n.prev }🤖 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/big_sorted_map/sources/big_sorted_map.move` around lines 348 - 355, `leaf_next` and `leaf_prev` currently return sibling links without verifying the node kind, so they can expose stale inner-node pointers instead of failing. Update both helpers in `big_sorted_map.move` to check `n.is_leaf` first and abort with `EWrongNodeKind` when called on an inner node, keeping the public cursor contract enforced through `leaf_next`/`leaf_prev` and the `Node` accessors.collections/sorted_set/examples/sorted_set/tests/unlock_queue_tests.move-87-93 (1)
87-93: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winEnd the expected-failure path on the aborting call.
unlock_queue::process_earliest(&mut q)is the intended failure boundary here, so the trailingts::return_shared(q)should be removed. The styleguide explicitly asks expected-failure tests not to include cleanup after the abort point.As per path instructions, "No cleanup in expected-failure tests; abort at the failure boundary." Based on learnings, "Follow
STYLEGUIDE.mdexactly for Move code style ... testing."Suggested cleanup
{ let mut q = ts::take_shared<UnlockQueue>(&scenario); unlock_queue::process_earliest(&mut q); // aborts here - ts::return_shared(q); // unreachable; satisfies the type checker };🤖 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/sorted_set/examples/sorted_set/tests/unlock_queue_tests.move` around lines 87 - 93, The expected-failure path in the unlock queue test should end at the aborting call itself, so remove the trailing cleanup after unlock_queue::process_earliest in the unlock_queue::process_earliest test block. Keep the failure boundary at the abort point and do not add any post-abort ts::return_shared(q) or similar cleanup in this expected-failure case.Sources: Path instructions, Learnings
collections/sorted_set/examples/sorted_set/tests/allowlist_tests.move-94-100 (1)
94-100: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStop this test at the abort boundary.
The
expected_failurecase should end onallowlist::approve_strict(&mut list, 1). Keepingts::return_to_senderafter the known abort point makes the example diverge from the repo’s required test pattern.As per path instructions, "No cleanup in expected-failure tests; abort at the failure boundary." Based on learnings, "Follow
STYLEGUIDE.mdexactly for Move code style ... testing."Suggested cleanup
{ let mut list = ts::take_from_sender<Allowlist>(&scenario); allowlist::approve_strict(&mut list, 1); // aborts here: 1 is already approved - ts::return_to_sender(&scenario, list); // unreachable; satisfies the type checker };🤖 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/sorted_set/examples/sorted_set/tests/allowlist_tests.move` around lines 94 - 100, The expected-failure test should stop immediately at the known abort in allowlist::approve_strict and not include any unreachable cleanup afterward. Update the Tx2 block in allowlist_tests.move so the failure boundary is the approve_strict call on the duplicate id, and remove the trailing ts::return_to_sender pattern from this expected_failure example to match the repo test style.Sources: Path instructions, Learnings
🧹 Nitpick comments (9)
collections/big_sorted_map/tests/bridge_tests.move (1)
21-184: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffModule-qualified calls instead of receiver syntax throughout.
Calls like
bsm::length(&map),sm::length(&back),bsm::head(&map),bsm::destroy_empty(map)usemodule::fn(obj, ...)form instead ofobj.fn(...). The productionbig_sorted_map.moveitself uses receiver syntax (result.append(leaf),inner.destroy_empty()), and STYLEGUIDE explicitly extends the receiver-syntax preference to tests. This same pattern recurs identically acrossconservation_tests.move,cursor_tests.move,differential_tests.move,structural_tests.move, andtype_tests.movein this cohort — flagging once here as the representative instance rather than repeating per file.Given the scale (hundreds of call sites across 6 large files) versus the purely stylistic nature of the fix, this is a good-to-have cleanup rather than a blocking issue.
As per path instructions: "Receiver syntax: prefer
obj.fn(...)overmodule::fn(obj, ...)for functions whose first param is the object (also in tests)."🤖 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/big_sorted_map/tests/bridge_tests.move` around lines 21 - 184, The test code uses module-qualified calls instead of the preferred receiver syntax, which is inconsistent with the style guide. Update the affected calls in this bridge test to use receiver form on the object values, matching the patterns used in big_sorted_map.move and other tests; focus on symbols like bsm::length, sm::length, bsm::head, bsm::tail, bsm::destroy_empty, and related helpers throughout the file. Apply the same receiver-syntax cleanup consistently wherever the first argument is the object being operated on.Source: Path instructions
collections/sorted_map/sources/sorted_map.move (1)
72-170: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAlign this production module with the repo’s canonical section/doc layout.
This new module collapses multiple STYLEGUIDE sections together, and several public APIs in this block (
new,length,is_empty,head,tail) still omit the structured#### Parameters/#### Returnsdocs the guide requires. Please normalize the section headers and API docs here before this becomes the reference implementation.As per path instructions,
Follow exact file section ordering using // === <Name> === delimitersandDocumentation: /// doc-comments for public APIs; include #### Parameters, #### Returns, and #### Aborts whenever relevant, listing every abort cause precisely.🤖 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/sorted_map/sources/sorted_map.move` around lines 72 - 170, The module header and public API docs in SortedMap need to be normalized to the repo’s canonical section/documentation layout. Reorganize the section delimiters in sorted_map.move to match the required // === <Name> === ordering, and update the public functions new, length, is_empty, head, tail, and destroy_empty to use structured /// docs with #### Parameters, #### Returns, and #### Aborts where applicable. Keep the docs precise and complete, and reference the existing symbols SortedMap, new, destroy_empty, length, is_empty, head, and tail while updating.Source: Path instructions
collections/big_sorted_map/examples/big_sorted_map/tests/clmm_pool_tests.move (1)
104-117: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEnd the expected-failure scenario at the aborting call.
The trailing
ts::end(scenario)is dead code, and the style guide asks these tests to stop at the failure boundary with no cleanup afterward. As per path instructions, "No cleanup in expected-failure tests; abort at the failure boundary."🤖 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/big_sorted_map/examples/big_sorted_map/tests/clmm_pool_tests.move` around lines 104 - 117, The expected-failure test deploy_below_degree_floor_aborts should stop at the aborting call and not include any cleanup afterward. Remove the trailing ts::end(scenario) after clmm_pool::deploy_and_share so the test ends at the failure boundary, consistent with the expected_failure style used in clmm_pool_tests.move and similar tests.Source: Path instructions
collections/big_sorted_map/examples/big_sorted_map/tests/order_book_tests.move (1)
133-155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the unreachable cleanup from the expected-failure tests.
Both failure scenarios already end at the library abort. The trailing
ts::end(scenario)lines only add dead cleanup code that the style guide explicitly asks to avoid in expected-failure tests. As per path instructions, "No cleanup in expected-failure tests; abort at the failure boundary."Also applies to: 163-180
🤖 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/big_sorted_map/examples/big_sorted_map/tests/order_book_tests.move` around lines 133 - 155, Remove the unreachable cleanup from the expected-failure tests in order_book_tests.move: both `close_nonempty_book_aborts` and the other failure case should end at the abort site instead of calling `ts::end(scenario)` afterward. Keep the test focused on the failing operation (`order_book::close` / the library abort boundary) and delete the dead cleanup code so the expected-failure style remains consistent. Ensure the test bodies still set up the scenario and trigger the abort, but do not add any post-abort teardown.Source: Path instructions
collections/big_sorted_map/tests/abort_tests.move (1)
47-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStop expected-failure tests at the aborting call.
These cases all leave unreachable cleanup after the line that is supposed to abort. Please end each test at the failure boundary instead of draining/destroying afterward. As per path instructions,
STYLEGUIDE.mdsays: “No cleanup in expected-failure tests; abort at the failure boundary.”Also applies to: 62-63, 85-86, 102-103, 118-119, 132-133, 148-150, 165-166, 179-180, 202-205, 222-223, 239-240, 265-266, 280-282
🤖 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/big_sorted_map/tests/abort_tests.move` around lines 47 - 48, The expected-failure tests in abort_tests.move continue past the aborting call with unreachable cleanup, so stop each test at the failure boundary. Update the relevant test cases around u::get, u::remove, and similar abort-triggering calls to end immediately after the call that is meant to abort, and remove any following u::drain_destroy or other cleanup in those tests. Use the test functions in abort_tests.move as the fix points and keep the failure-only flow with no post-abort cleanup, per the expected-failure test pattern.Sources: Path instructions, Learnings
collections/sorted_map/tests/differential_tests.move (1)
60-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
assert_eq!for these invariant checks.This file otherwise follows the test pattern well, but these boolean assertions still miss the repo’s required assertion style. As per path instructions, "Use
std::unit_test::assert_eqfor assertions (avoid abort-code arithmetic inassert!)."Also applies to: 88-88
🤖 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/sorted_map/tests/differential_tests.move` at line 60, The invariant checks in the sorted map differential tests are using boolean assert! instead of the required std::unit_test::assert_eq style. Update the assertions in differential_tests.move, including the wf check in the test loop and the other matching occurrence, to compare the condition against true with assert_eq! so the tests follow the repo’s assertion pattern.Source: Path instructions
collections/sorted_map/tests/type_tests.move (1)
80-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
assert_eq!for the independence checks too.These are straightforward assertions, but the repo’s test style asks for
std::unit_test::assert_eqrather than booleanassert!. As per path instructions, "Usestd::unit_test::assert_eqfor assertions (avoid abort-code arithmetic inassert!)."🤖 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/sorted_map/tests/type_tests.move` around lines 80 - 82, The test assertions in the type tests use boolean `assert!` for simple equality checks, but the repo style prefers `std::unit_test::assert_eq`. Update the assertions in `type_tests.move` near the `u::get`, `sm::length`, and `sm::is_empty` checks to use `assert_eq!` instead of `assert!`, keeping the same expectations and using the existing `u`, `sm`, `a`, and `b` symbols.Source: Path instructions
collections/sorted_map/tests/structural_tests.move (1)
33-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit these grouped
assert!checks intoassert_eq!assertions.Besides matching the repo convention, breaking the conjunctions apart will make the failing invariant obvious when one of these structural cases regresses. As per path instructions, "Use
std::unit_test::assert_eqfor assertions (avoid abort-code arithmetic inassert!)."Also applies to: 48-48, 58-58, 67-67, 126-128, 161-169, 188-188, 200-200
🤖 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/sorted_map/tests/structural_tests.move` around lines 33 - 38, Replace the grouped boolean assert in the sorted_map structural tests with separate std::unit_test::assert_eq assertions, so each invariant in the split/retained map checks is reported independently. Update the assertions around the relevant structural helpers (sm::length, u::wf, sm::head, sm::tail, and u::get) to compare each expected value directly instead of combining them with && inside assert!, and apply the same assert_eq-style refactor to the other listed cases in this test file.Source: Path instructions
collections/sorted_map/tests/conservation_tests.move (1)
98-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
assert_eq!instead of booleanassert!here.These checks drift from the repo’s test convention, and the compound boolean also makes failures less local than separate assertions. As per path instructions, "Use
std::unit_test::assert_eqfor assertions (avoid abort-code arithmetic inassert!)."🤖 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/sorted_map/tests/conservation_tests.move` around lines 98 - 103, Replace the compound boolean check in the conservation test with `assert_eq!`-style assertions to match the repo’s unit test convention and keep failures local. In `conservation_tests.move`, update the final check that uses `u::nd_value_id` so it is split into separate `assert_eq!` assertions (or equivalent `std::unit_test::assert_eq` usage) rather than a single `assert!`, keeping the existing `sm::head`, `sm::tail`, and `sm::length` checks unchanged.Source: Path instructions
| use sui::coin::{Self, Coin}; | ||
| use sui::sui::SUI; | ||
| use sui::test_scenario as ts; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,220p' collections/sorted_map/tests/conservation_tests.moveRepository: OpenZeppelin/contracts-sui
Length of output: 6919
🏁 Script executed:
rg -n -C 2 'tx_context::dummy\(|object::new\(|transfer::share_object\(|use sui::tx_context|use sui::object|use sui::transfer|UID' collections . -g '*.move'Repository: OpenZeppelin/contracts-sui
Length of output: 50385
🏁 Script executed:
rg -n -C 1 'option::some\(' collections -g '*.move' | sed -n '1,120p'Repository: OpenZeppelin/contracts-sui
Length of output: 10689
🏁 Script executed:
sed -n '1,30p' collections/sorted_set/tests/type_tests.moveRepository: OpenZeppelin/contracts-sui
Length of output: 1601
Add the missing imports for the test helpers.
tx_context::dummy(), UID, object::new, and transfer::share_object are used later in this file, but they aren’t brought into scope here, so this test module won’t compile.
Proposed fix
use std::unit_test::assert_eq;
use sui::coin::{Self, Coin};
+use sui::object::{Self as object, UID};
use sui::sui::SUI;
use sui::test_scenario as ts;
+use sui::transfer;
+use sui::tx_context;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| use sui::coin::{Self, Coin}; | |
| use sui::sui::SUI; | |
| use sui::test_scenario as ts; | |
| use sui::coin::{Self, Coin}; | |
| use sui::object::{Self as object, UID}; | |
| use sui::sui::SUI; | |
| use sui::test_scenario as ts; | |
| use sui::transfer; | |
| use sui::tx_context; |
🤖 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/sorted_map/tests/conservation_tests.move` around lines 13 - 15,
Add the missing imports at the top of the test module so the later uses of
tx_context::dummy(), UID, object::new, and transfer::share_object resolve
correctly. Update the import block in conservation_tests.move to bring those
helpers into scope alongside the existing sui::coin and sui::test_scenario
imports, using the same style as the surrounding module.
|
2 participants
Resolves #261
Resolves #118
Resolves #321
These three ordered-collection packages ship as one PR since they will be
audited together (sorted_set and big_sorted_map both build on sorted_map).
Line coverage: sorted_map and sorted_set 100%, big_sorted_map >=99%.
PR Checklist
Summary by CodeRabbit
New Features
Documentation
Bug Fixes