Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 55 minutes and 37 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (65)
WalkthroughMigrates persistent in-memory tree implementations from AVL ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Member
B+ Tree Strengths to LeverageAfter migration, these patterns become cheaper — simplify any prior workarounds:
Check-1: Is NewBPTree32() the Right Default?Fanout 32 is a starting point, not an answer. Rules of thumb:
Check-2: Benchmark ValidationNewBPTree32() must be validated — not assumed — on hot-path / large trees:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
contract/r/gnoswap/gov/staker/v1/util_test.gno (1)
142-142:⚠️ Potential issue | 🟡 MinorUpdate stale AVL wording in the test comments.
These comments still describe AVL trees even though the fixtures now use B+Tree.
Proposed comment cleanup
-// TestUtil_getUint64FromTree tests retrieving uint64 from AVL tree +// TestUtil_getUint64FromTree tests retrieving uint64 from B+Tree @@ -// TestUtil_updateUint64InTree tests updating uint64 in AVL tree +// TestUtil_updateUint64InTree tests updating uint64 in B+Tree @@ -// TestUtil_getOrCreateInnerTree tests getting or creating inner AVL tree +// TestUtil_getOrCreateInnerTree tests getting or creating inner B+TreeAlso applies to: 207-207, 301-301
contract/r/gnoswap/staker/v1/calculate_pool_position_reward.gno (1)
175-181:⚠️ Potential issue | 🟡 MinorStale inline comment.
The comment at Line 176 still describes the value as a slice of incentive IDs, but the 2-level structure (per the PR-wide migration and the test setup in
external_incentive_test.gno) stores a*bptree.BPTreekeyed bypoolPath→[]stringat each timestamp.📝 Proposed fix
incentivesByTime.Iterate(startTime, endTime, func(_ int64, value any) bool { - // Value is a slice of incentive IDs (handles timestamp collisions) + // Value is a B+Tree keyed by poolPath → []string (handles same-timestamp, multi-pool incentives) poolIncentiveIds, ok := value.(*bptree.BPTree)contract/r/gnoswap/staker/v1/external_incentive_test.gno (1)
1006-1006:⚠️ Potential issue | 🟡 MinorStale "AVL Tree" reference in comment.
After the migration this no longer refers to an AVL tree. Consider generalizing the comment (or just saying "set semantics").
📝 Proposed fix
- // Verify it's only added once (AVL Tree property) + // Verify it's only added once (set semantics on underlying tree)contract/p/gnoswap/version_manager/version_manager.gno (2)
168-173:⚠️ Potential issue | 🟡 MinorStale "AVL tree" reference in doc comment.
The backing store is now a B+Tree; update the doc for accuracy.
📝 Proposed fix
-// GetInitializers returns the AVL tree containing all registered initializer functions. +// GetInitializers returns the B+Tree containing all registered initializer functions. // Keys are package paths, values are initializer functions. // Useful for inspecting which versions are available.
243-245:⚠️ Potential issue | 🟡 MinorStale "AVL tree" reference in doc comment.
Same as above — please update to reflect the B+Tree backing.
📝 Proposed fix
// makeInitializerSafe creates a safe copy of an initializer function for read-only tree access. -// This is used by GetInitializers to wrap the internal AVL tree in a read-only view, +// This is used by GetInitializers to wrap the internal B+Tree in a read-only view, // preventing external modification of registered initializers.contract/r/gnoswap/staker/v1/reward_calculation_types.gno (1)
68-85:⚠️ Potential issue | 🟡 MinorUpdate the
UintTreedoc comment to B+Tree.Line 68 still says this wrapper is around an AVL tree, but the field and constructor now use
*bptree.BPTree.Proposed fix
-// UintTree is a wrapper around an AVL tree for storing block timestamps as strings. +// UintTree is a wrapper around a B+Tree for storing block timestamps as strings.
🧹 Nitpick comments (6)
contract/r/gnoswap/staker/tree.gno (1)
11-28: Update the stale AVL wording inUintTreedocs.The implementation now wraps
*bptree.BPTree, but the exported type comment still says “AVL tree.”Documentation cleanup
-// UintTree is a wrapper around an AVL tree for storing block timestamps as strings. +// UintTree is a wrapper around a B+Tree for storing block timestamps as strings.contract/r/gnoswap/gov/staker/tree.gno (1)
11-28: Update the stale AVL wording inUintTreedocs.The implementation now wraps
*bptree.BPTree, but the exported type comment still says “AVL tree.”Documentation cleanup
-// UintTree is a wrapper around an AVL tree for storing block timestamps as strings. +// UintTree is a wrapper around a B+Tree for storing block timestamps as strings.contract/r/gnoswap/staker/pool.gno (1)
213-228: Refresh theIncentivesfield documentation.Line 217 still documents the backing store as AVL, but Line 228 now uses
*bptree.BPTree.Documentation cleanup
-// - incentives: AVL tree storing ExternalIncentive objects indexed by incentiveId +// - incentives: B+Tree storing ExternalIncentive objects indexed by incentiveIdcontract/r/gnoswap/gov/staker/v1/util_test.gno (1)
345-345: Use the table’s expected panic message.Line 345 hardcodes the B+Tree cast message, so future cases could set
expectedPanicMessageand still assert the wrong value.Proposed test cleanup
- uassert.PanicsWithMessage(t, "failed to cast value to *bptree.BPTree: uint64", func() { + uassert.PanicsWithMessage(t, tc.expectedPanicMessage, func() {contract/r/gnoswap/referral/keeper.gno (1)
14-19: Update stale doc comment referring to AVL.The struct doc comment still says "using AVL tree storage" but the backing store is now
*bptree.BPTree. Update to avoid misleading future readers.📝 Proposed fix
-// keeper implements ReferralKeeper using AVL tree storage. -// It includes rate limiting to prevent abuse. +// keeper implements ReferralKeeper using B+Tree storage. +// It includes rate limiting to prevent abuse. type keeper struct { store *bptree.BPTree // address(string) -> referral address(string) lastOps *bptree.BPTree // address(string) -> last operation timestamp(int64) }contract/r/gnoswap/protocol_fee/store.gno (1)
7-7: LGTM — KV-backed B+Tree migration is consistent across all four accumulators.Note: the redundant
s.kvStore.Set(StoreKey..., tree)at the end of eachSet*Itemmethod (e.g., lines 109, 157, 205, 253) is harmless sincekvStore.GetTreereturns a pointer and mutations already persist, but it’s preserved from the pre-migration shape and probably worth a follow-up cleanup in a separate PR rather than mixing in here.Also applies to: 70-70, 73-73, 118-118, 121-121, 166-166, 169-169, 214-214, 217-217
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d8f54f57-5ba7-40d4-bebd-367275ca926d
📒 Files selected for processing (101)
contract/p/gnoswap/store/doc.gnocontract/p/gnoswap/store/kv_store.gnocontract/p/gnoswap/store/kv_store_test.gnocontract/p/gnoswap/store/types.gnocontract/p/gnoswap/store/utils.gnocontract/p/gnoswap/store/utils_test.gnocontract/p/gnoswap/version_manager/doc.gnocontract/p/gnoswap/version_manager/types.gnocontract/p/gnoswap/version_manager/version_manager.gnocontract/p/gnoswap/version_manager/version_manager_test.gnocontract/r/gnoswap/gov/governance/store.gnocontract/r/gnoswap/gov/governance/store_test.gnocontract/r/gnoswap/gov/governance/types.gnocontract/r/gnoswap/gov/governance/v1/_helper_test.gnocontract/r/gnoswap/gov/governance/v1/_mock_test.gnocontract/r/gnoswap/gov/governance/v1/governance_propose.gnocontract/r/gnoswap/gov/governance/v1/governance_snapshot_test.gnocontract/r/gnoswap/gov/governance/v1/governance_vote_test.gnocontract/r/gnoswap/gov/governance/v1/init.gnocontract/r/gnoswap/gov/governance/v1/instance.gnocontract/r/gnoswap/gov/governance/v1/parameter_registry.gnocontract/r/gnoswap/gov/staker/delegation_manager.gnocontract/r/gnoswap/gov/staker/delegation_manager_test.gnocontract/r/gnoswap/gov/staker/emission_reward_manager.gnocontract/r/gnoswap/gov/staker/launchpad_project_deposits.gnocontract/r/gnoswap/gov/staker/launchpad_project_deposits_test.gnocontract/r/gnoswap/gov/staker/protocol_fee_reward_manager.gnocontract/r/gnoswap/gov/staker/protocol_fee_reward_manager_test.gnocontract/r/gnoswap/gov/staker/store.gnocontract/r/gnoswap/gov/staker/store_test.gnocontract/r/gnoswap/gov/staker/tree.gnocontract/r/gnoswap/gov/staker/types.gnocontract/r/gnoswap/gov/staker/v1/_mock_test.gnocontract/r/gnoswap/gov/staker/v1/getter.gnocontract/r/gnoswap/gov/staker/v1/init.gnocontract/r/gnoswap/gov/staker/v1/staker_delegate_test.gnocontract/r/gnoswap/gov/staker/v1/staker_delegation_snapshot.gnocontract/r/gnoswap/gov/staker/v1/staker_delegation_snapshot_cleanup_test.gnocontract/r/gnoswap/gov/staker/v1/staker_delegation_snapshot_test.gnocontract/r/gnoswap/gov/staker/v1/state.gnocontract/r/gnoswap/gov/staker/v1/state_delegation_history_test.gnocontract/r/gnoswap/gov/staker/v1/state_test.gnocontract/r/gnoswap/gov/staker/v1/util.gnocontract/r/gnoswap/gov/staker/v1/util_test.gnocontract/r/gnoswap/launchpad/getter_test.gnocontract/r/gnoswap/launchpad/reward_manager.gnocontract/r/gnoswap/launchpad/store.gnocontract/r/gnoswap/launchpad/store_test.gnocontract/r/gnoswap/launchpad/types.gnocontract/r/gnoswap/launchpad/v1/_helper_test.gnocontract/r/gnoswap/launchpad/v1/init.gnocontract/r/gnoswap/launchpad/v1/launchpad_project_test.gnocontract/r/gnoswap/launchpad/v1/launchpad_reward_test.gnocontract/r/gnoswap/launchpad/v1/reward_manager_test.gnocontract/r/gnoswap/mock/pool_store.gnocontract/r/gnoswap/mock/position_store.gnocontract/r/gnoswap/pool/getter_utils.gnocontract/r/gnoswap/pool/pool.gnocontract/r/gnoswap/pool/pool_test.gnocontract/r/gnoswap/pool/store.gnocontract/r/gnoswap/pool/store_test.gnocontract/r/gnoswap/pool/types.gnocontract/r/gnoswap/pool/v1/_helper_test.gnocontract/r/gnoswap/pool/v1/getter_test.gnocontract/r/gnoswap/pool/v1/init.gnocontract/r/gnoswap/pool/v1/swap_test.gnocontract/r/gnoswap/pool/v1/tick_test.gnocontract/r/gnoswap/position/store.gnocontract/r/gnoswap/position/store_test.gnocontract/r/gnoswap/position/types.gnocontract/r/gnoswap/position/v1/_mock_test.gnocontract/r/gnoswap/position/v1/init.gnocontract/r/gnoswap/protocol_fee/store.gnocontract/r/gnoswap/protocol_fee/types.gnocontract/r/gnoswap/protocol_fee/v1/_mock_test.gnocontract/r/gnoswap/protocol_fee/v1/protocol_fee_state.gnocontract/r/gnoswap/protocol_fee/v1/protocol_fee_state_test.gnocontract/r/gnoswap/referral/keeper.gnocontract/r/gnoswap/router/v1/_mock_test.gnocontract/r/gnoswap/staker/pool.gnocontract/r/gnoswap/staker/store.gnocontract/r/gnoswap/staker/store_test.gnocontract/r/gnoswap/staker/tree.gnocontract/r/gnoswap/staker/types.gnocontract/r/gnoswap/staker/v1/_mock_test.gnocontract/r/gnoswap/staker/v1/calculate_pool_position_reward.gnocontract/r/gnoswap/staker/v1/external_incentive.gnocontract/r/gnoswap/staker/v1/external_incentive_test.gnocontract/r/gnoswap/staker/v1/getter_test.gnocontract/r/gnoswap/staker/v1/init.gnocontract/r/gnoswap/staker/v1/reward_calculation_incentives.gnocontract/r/gnoswap/staker/v1/reward_calculation_pool.gnocontract/r/gnoswap/staker/v1/reward_calculation_pool_tier.gnocontract/r/gnoswap/staker/v1/reward_calculation_pool_tier_test.gnocontract/r/gnoswap/staker/v1/reward_calculation_tick.gnocontract/r/gnoswap/staker/v1/reward_calculation_types.gnocontract/r/gnoswap/staker/v1/staker.gnocontract/r/gnoswap/staker/v1/staker_test.gnocontract/r/gnoswap/test/fuzz/_mock_test.gnocontract/r/scenario/upgrade/implements/mock/gov/governance/test_impl.gnocontract/r/scenario/upgrade/implements/v3_valid/launchpad/init.gno
Updates stale AVL references in comments, READMEs, and test annotations that the earlier docs pass did not cover. Also corrects setDistributionBpsPct comment, which described a map as an AVL tree. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
42 txtar files declared loadpkg gno.land/p/nt/avl/v0 but never reference the package; the dependency became dead after the AVL→B+Tree migration. The data_structure_gas_measurement fixture still uses avl and is left alone since reworking it requires re-measuring gas values. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaces avl.NewTree with bptree.NewBPTree32 and renames AVLxxx helpers to BPTreexxx so the fixture reflects the current tree implementation. B+Tree assertions use a 'GAS USED:' prefix placeholder; concrete values must be re-measured (run the txtar and paste the new gas numbers back). Map, slice, and struct assertions are unchanged since none of them touch the tree package. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Fills in B+Tree gas placeholders and refreshes every other entry with values captured by running the txtar through gno.land/pkg/integration. Map, slice, and struct measurements also shifted (unrelated to the tree swap — they track gno VM gas accounting as it stands today), so every expectation is re-blessed in the same pass. Observations worth keeping in mind: - B+Tree Insert/Get at 1000 items cost ~148M gas, roughly 5x the Map equivalent (~31M). Confirms Map stays preferable for pure point-query workloads where stable key-ordering is not needed. - B+Tree Iterate 100 cost ~14.4M vs Map Iterate 100 at ~2.8M, so the linked-list leaf walk still carries real overhead at this size; the ordered-iteration benefit only pays off where ordering is required. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Extends the data_structure benchmark to measure B+Tree at fanout 32/64/128 side by side so the Phase 2-4 tuning work has a concrete reference for "does raising fanout actually help this workload". All 49 GAS USED values are deterministic (re-verified with a second run). Key findings recorded in the fixture header: - N=10: fanout has no effect (tree fits in a single leaf) - N=100: 64 ≈ -8% vs 32, 128 ≈ -16% vs 32 (largest win range) - N=1000: 64 ≈ -8% vs 32, 128 ≈ -9% vs 32 (128 gain diminishes) Guidance: 32→64 is a near-free ~8% for any tree past a single leaf; 64→128 mostly pays off at the 100-ish item range. Small trees (N<32) stay on 32. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
dongwon8247
reviewed
Apr 24, 2026
Member
dongwon8247
left a comment
dongwon8247
left a comment
There was a problem hiding this comment.
will add more commits to open discussions on optimizing data types. please wait.
…timization targets Add persistent_data_structure_benchmark.txtar measuring per-operation gas on realm-persisted state with realistic struct values (8 fields, ~200 bytes). This is the authoritative evidence for BPTree vs Map decisions — the previous ephemeral benchmarks overstated Map's advantage by not accounting for serialization/deserialization cost. Key findings from persistent benchmark: - N<200: Map wins for Get (2.8x), Set (1.6x), and Add (1.7x) - N>=500: BPTree wins for Get (2.0x), Set (3.2x), and Add (3.0x) - Incremental add (the actual init pattern) follows the same crossover - Fanout 64 saves ~4% over 32 at N>=1000 Annotate 16 BPTree instances across 12 modules with TODO comments citing benchmark evidence: - 6 trees (N<200, no Iterate): recommend map conversion - 10 trees (N>=1000): recommend fanout 32→64 upgrade Also clarify the purpose header of data_structure_gas_measurement.txtar to distinguish it from the persistent benchmark (ephemeral fanout sweep vs authoritative decision reference). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Dongwon <74406335+dongwon8247@users.noreply.github.com>
Member
|
Can you check 3b08f75 to see if it's worth changing the data type? @notJoon @junghoon-vans @jinoosss |
The RBAC validator functions (updatableRoleNameValidator, removableRoleNameValidator) reference prbac.ROLE_ADMIN and prbac.IsSystemRole but the import was missing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit