Update scarb, starknet and l2 contract to latest version#111
Update scarb, starknet and l2 contract to latest version#111Ugo-X merged 4 commits intoExplore-Beyond-Innovations:mainfrom
Conversation
|
Warning Rate limit exceeded@JoE11-y has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 21 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (17)
WalkthroughTooling and dependency versions bumped for L2 contracts; Scarb manifest updated to enable CASM output. Source changes replace storage Vec append().write(...) with push(...) and adjust imports/feature flags to align with newer StarkNet toolchains. .gitignore updated to ignore Scarb.lock. Changes
Sequence Diagram(s)No sequence diagram — changes are tooling/imports and local storage API adaptations; control-flow remains unchanged. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
contracts/L2/src/dao/Timelock.cairo (1)
214-226: Bug: get_pending_actions iterates numeric IDs but actions are keyed by hashed IDs.action_id is a Poseidon-based hash (see generate_action_id), not a sequential counter. Iterating 0..action_count and probing self.actions.entry(action_id) will miss most actions and can read uninitialized storage paths. This can yield incorrect results or undefined/default reads.
Suggested approach: track action IDs in a storage Vec and iterate that when building the pending list. Minimal change:
- fn get_pending_actions(self: @TContractState) -> Array<felt252> { - let mut pending = ArrayTrait::new(); - let count = self.action_count.read(); - - for i in 0..count { - let action_id: felt252 = i.into(); - let action_entry = self.actions.entry(action_id); - if action_entry.status.read() == ActionStatus::Pending { - pending.append(action_id); - } - }; - pending - } + fn get_pending_actions(self: @TContractState) -> Array<felt252> { + let mut pending = ArrayTrait::new(); + let ids_len = self.pending_ids.len(); + for i in 0..ids_len { + let action_id = self.pending_ids.at(i).read(); + let entry = self.actions.entry(action_id); + if entry.status.read() == ActionStatus::Pending { + pending.append(action_id); + } + }; + pending + }Additional changes needed outside this range:
- Add a storage Vec to track IDs and populate it when queuing actions.
// In Storage pending_ids: Vec<felt252>,// In queue_action, after setting status to Pending: self.pending_ids.push(action_id);Optionally, you can later compact pending_ids or keep it append-only and filter by status as shown.
contracts/L2/src/dao/DAO.cairo (1)
285-289: Poll voting is effectively disabled due to incorrect phase checks.
vote_in_polland_is_in_poll_phaseboth requirePollPassed, but the voting phase isPollActive. As written, no one can vote during the poll.Update
_is_in_poll_phaseto checkPollActiveand time, and (optionally) align/remove the redundant direct status assert invote_in_poll.Apply this diff to fix
_is_in_poll_phase:- fn _is_in_poll_phase(self: @ContractState, proposal_id: u256) -> bool { - let proposal = self.proposals.read(proposal_id); - let current_time = get_block_timestamp(); - proposal.status == ProposalStatus::PollPassed && current_time <= proposal.poll_end_time - } + fn _is_in_poll_phase(self: @ContractState, proposal_id: u256) -> bool { + let proposal = self.proposals.read(proposal_id); + let current_time = get_block_timestamp(); + proposal.status == ProposalStatus::PollActive && current_time <= proposal.poll_end_time + }If you prefer to keep a direct status check in
vote_in_poll, change it toPollActive(or remove it, see next comment).Also applies to: 520-524
🧹 Nitpick comments (4)
contracts/L2/.gitignore (1)
5-6: Consider committing Scarb.lock for reproducible builds.Ignoring Scarb.lock makes dependency resolution non-deterministic across dev/CI, which is risky when upgrading toolchains. Most contract repos commit Scarb.lock to ensure consistent compilation and bytecode reproducibility.
Apply this diff to stop ignoring the lockfile:
.tool-versions node_modules -.env -Scarb.lock +.envIf you prefer to keep it ignored, please document the rationale in CONTRIBUTING.md and pin exact dependency versions in Scarb.toml to reduce drift.
contracts/L2/src/dao/DAO.cairo (3)
281-294: Remove duplicate asserts and single-source vote count updates.
assert(!self.has_voted.read(...), 'Already voted')is present twice; keep one.- You update vote counts both via
_update_vote_counts(...)and again on the localproposalcopy. This results in two RMW cycles and can be simplified to a single source of truth (prefer the internal helper).Minimal change to deduplicate the "Already voted" assert:
fn vote_in_poll(ref self: ContractState, proposal_id: u256, support: bool) { let caller = get_caller_address(); let mut proposal = self._validate_proposal_exists(proposal_id); assert(self._is_in_poll_phase(proposal_id), 'Not in poll phase'); - assert(!self.has_voted.read((proposal_id, caller)), 'Already voted'); assert(proposal.id == proposal_id, 'Proposal does not exist'); - assert(proposal.status == ProposalStatus::PollPassed, 'Not in poll phase'); let current_time = get_block_timestamp(); assert(current_time <= proposal.poll_end_time, 'Poll phase ended'); assert(!self.has_voted.read((proposal_id, caller)), 'Already voted'); let vote_weight = self._get_voter_weight(caller); assert(vote_weight > 0, 'No voting power'); self._update_vote_counts(proposal_id, support, vote_weight); - if support { - proposal.vote_for += vote_weight; - } else { - proposal.vote_against += vote_weight; - } self.proposals.write(proposal_id, proposal);If you want, I can follow up with a patch that keeps all counting inside
_update_vote_countsand only persists non-count fields from the localproposalhere.
243-245: Nit: fix assertion message grammar.
Use “Binding vote already cast” (lowercase ‘vote’, ‘cast’ vs ‘casted’).Apply this diff:
- assert!(binding_vote.get_vote().is_none(), "Binding Vote Already casted"); + assert!(binding_vote.get_vote().is_none(), "Binding vote already cast");
113-115: Deprecatecontract_address_constand remove the deprecated StarkNet consts featureWe’re currently relying on
#[feature("deprecated-starknet-consts")]andcontract_address_constin production and tests, which is a temporary workaround that will break under future toolchains. To future-proof the DAO:• Production code
- contracts/L2/src/dao/DAO.cairo: lines 340, 371 – both initialize
executive_action_addressviacontract_address_const::<0x0>().• Test suite
- contracts/L2/tests/**: dozens of calls to
contract_address_const::<'…'>()across tests (e.g. test_L2Oracle.cairo, test_xZBERC20.cairo, test_Dynamicrate.cairo, test_ZeroXBridgeL2.cairo, test_DAO.cairo).Refactor suggestion:
- In your
Proposalstruct, changetopub executive_action_address: ContractAddresspub executive_action_address: Option<ContractAddress>- In
create_proposal/submit_proposal, setexecutive_action_address: None.- In
start_binding_vote, assignexecutive_action_address = Some(execute_action_address).- Update all current call sites (production & tests) to pass explicit addresses instead of using
contract_address_const.This change will let you drop the deprecated feature gate and remove all
contract_address_constusages. Let me know if you’d like a full patch for the struct and affected call sites.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
contracts/L2/Scarb.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
contracts/L2/.gitignore(1 hunks)contracts/L2/.tool-versions(1 hunks)contracts/L2/Scarb.toml(1 hunks)contracts/L2/src/core/ZeroXBridgeL2.cairo(1 hunks)contracts/L2/src/dao/DAO.cairo(2 hunks)contracts/L2/src/dao/Timelock.cairo(1 hunks)contracts/L2/src/mocks/MockMerkleManager.cairo(1 hunks)
🔇 Additional comments (10)
contracts/L2/.tool-versions (1)
1-2: Version bumps align with PR goals; looks good.Upgrading to scarb 2.12.0 and starknet-foundry 0.48.1 is consistent with the stated objectives and recent ecosystem changes.
contracts/L2/src/mocks/MockMerkleManager.cairo (1)
98-98: Correct use of storage Vec.push with newer StarkNet toolchains.Replacing append().write(...) with push(...) for storage Vec is the right API on recent Cairo/StarkNet versions. Semantics and gas profile are appropriate.
contracts/L2/src/dao/Timelock.cairo (1)
133-137: Good modernization: storage Vec.push is the correct pattern.Using push to copy calldata into the storage Vec is correct and clearer than append().write with the updated storage API.
contracts/L2/src/core/ZeroXBridgeL2.cairo (2)
464-466: Correct migration to storage Vec.push.Using push for appending the tail of peaks is the proper API in updated Cairo/StarkNet. The overwrite-then-append logic remains intact.
404-410: Inconsistent semantics vs. Mock: zeros are retained here but filtered in Mock get_last_peaks.This implementation returns all stored peaks including zeros, whereas MockMerkleManager.get_last_peaks filters out zeros before returning. If MMR.append expects only the active peaks, consider normalizing both to the same behavior to avoid subtle divergence across environments.
Would you like me to harmonize both implementations to filter non-zero peaks before passing to MMR and emitting events?
contracts/L2/src/dao/DAO.cairo (2)
1-1: Import consolidation aligns with Starknet 2.12 APIs — looks good.
Usinguse starknet::{ContractAddress, storage::Map};at the file scope cleanly supports types used outside the module (e.g., Proposal/ProposalBindingData). No concerns.
119-121: Storage import path update is correct for v2.12.
Switching tostarknet::storage::{...}matches current module locations in the Starknet 2.12 toolchain.contracts/L2/Scarb.toml (3)
24-24: casm = true is a sensible addition.
Keeping bothsierra = trueandcasm = trueis recommended for deployment pipelines still consuming CASM artifacts. No action needed.
10-10: Confirm OpenZeppelin 0.20.0 compatibility
Starknet bumped to 2.12.0 as intended. Scarb.toml now includes:• starknet = "2.12.0"
• assert_macros = "2.12.0"
• snforge_std = "0.48.0"
• openzeppelin_access/introspection/token/upgrades/utils = "0.20.0".tool-versions also pins scarb 2.12.0 and starknet-foundry 0.48.1.
Please verify in your local build & test matrix that all OpenZeppelin 0.20.0 crates work correctly against Starknet 2.12.0.
18-20: Dev tooling bumps validatedWe’ve confirmed in contracts/L2/Scarb.toml (lines 18–20) that
• assert_macros = "2.12.0"
• snforge_std = "0.48.0"This aligns with Starknet Foundry 0.48.x. If you pin Foundry to 0.48.1, bump snforge_std to 0.48.1 for strict symmetry; otherwise 0.48.0 is acceptable.
Please verify:
- Your local .tool-versions (or asdf config) for the Foundry version
- That CI’s snforge installation matches the expected version
JoE11-y
left a comment
JoE11-y
left a comment
There was a problem hiding this comment.
Thanks for this @GideonBature
|
You are welcome. Thank you for the opportunity to keep contributing. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
contracts/L2/src/dao/Timelock.cairo (2)
257-266: High-bit truncation bug in u256 hashing causes information loss.Masking
highwith0x0FFFFFFFFFFFFFFFFretains only 64 bits (not 124 as the comment suggests), introducing avoidable collisions when compressing a u256. Bothlowandhigh(u128 each) fit into felt252 as separate field elements and do not require truncation before hashing.Fix: hash
highandlowdirectly without masking.Apply this diff:
- // Truncate the high part to 124 bits (since felt252 can hold 252 bits: 128 + 124). - let truncated_high = high - & 0x0FFFFFFFFFFFFFFFF; // Mask to retain only the lower 124 bits. - - // Use Poseidon hashing to combine the truncated high part and low part into a single - // felt252. - PoseidonTrait::new() - .update(truncated_high.into()) // Convert truncated_high to felt252. - .update(low.into()) // Convert low to felt252. - .finalize() // Finalize the hash to produce a single felt252. + // Hash the (high, low) limbs directly; both fit into felt252 without truncation. + PoseidonTrait::new() + .update(high.into()) + .update(low.into()) + .finalize()Optional tightening: Instead of per-element hashing via
u256_to_felt252, flatten each u256 into two felts and hash the entire flattened span ingenerate_action_id. I can provide a small refactor if preferred.
211-223: get_pending_actions incorrectly enumerates action IDs
The current implementation loops0..action_count, converts each integer to a felt, and treats that as an action ID—but real IDs are Poseidon hashes, so this will never find the queued actions. You must explicitly track and iterate the actual IDs.Please update
contracts/L2/src/dao/Timelock.cairoas follows:• In the
Storagestruct (around line 36), add aVec<felt252>to record IDs:struct Storage { #[starknet::storage_node] actions: Map<felt252, ActionNode>, - action_count: u64, + action_count: u64, + #[starknet::storage_node] + action_ids: Vec<felt252>, minimum_delay: u64, governance: ContractAddress, }• In
queue_action(around line 136), after writing the new entry and before incrementingaction_count, push the ID:action_entry.status.write(ActionStatus::Pending); + self.action_ids.push(action_id); self.action_count.write(self.action_count.read() + 1);• Rewrite
get_pending_actions(lines 211–223) to iterate overaction_idsinstead of0..action_count:- fn get_pending_actions(self: @ContractState) -> Array<felt252> { - let mut pending = ArrayTrait::new(); - let count = self.action_count.read(); - - for i in 0..count { - let action_id: felt252 = i.into(); - let action_entry = self.actions.entry(action_id); - if action_entry.status.read() == ActionStatus::Pending { - pending.append(action_id); - } - } - pending - } + fn get_pending_actions(self: @ContractState) -> Array<felt252> { + let mut pending = ArrayTrait::new(); + let count = self.action_ids.len(); + let mut i = 0; + while i < count { + let action_id = self.action_ids.at(i).read(); + let action_entry = self.actions.entry(action_id); + if action_entry.status.read() == ActionStatus::Pending { + pending.append(action_id); + } + i = i + 1; + } + pending + }These changes ensure you enumerate exactly the IDs you’ve queued, then filter by status.
🧹 Nitpick comments (4)
contracts/L2/src/core/ZeroXBridgeL2.cairo (3)
311-321: Consider a graceful fallback when TVL is zero to avoid reverting during bootstrap.Currently, total TVL of zero will cause an early assert failure, blocking mint/burn paths if the oracle hasn’t reported yet. Returning the baseline PRECISION when TVL is zero avoids dividing by zero, keeps behavior consistent with the “raw_rate == 0” case, and eases network bootstrap.
Proposed change:
- assert(total_tvl > 0, 'TVL cannot be zero'); + // Avoid division by zero during bootstrap/initialization. + if total_tvl == 0 { + return PRECISION; + }Please confirm this aligns with product expectations and oracle behavior (i.e., whether zero TVL should pause operations or allow a default rate). If zero should be a hard stop, the current assert is fine.
338-347: Remove redundant inner braces in set_rates.Minor cleanup for readability; the extra block scopes are unnecessary.
Apply this refactor:
- if let Option::Some(new_min) = min_rate { - { - current_rates.min_rate = new_min; - } - } + if let Option::Some(new_min) = min_rate { + current_rates.min_rate = new_min; + } - if let Option::Some(new_max) = max_rate { - { - current_rates.max_rate = new_max; - } - } + if let Option::Some(new_max) = max_rate { + current_rates.max_rate = new_max; + }
444-463: Switch to Vec.push is correct for newer storage Vec APIs; consider shrinking instead of zero-filling (optional).
- The replacement of append().write(...) with push(...) is the right API for the current toolchain.
- Optional: when peaks_len < last_peaks_len, zero-filling the tail preserves length. If the MMR implementation accepts a shorter peaks vector (no trailing zeros required), shrinking the storage Vec (e.g., via a truncate/pop operation if available) would:
- reduce storage reads/writes,
- avoid persisting trailing zeros,
- likely save gas.
If the storage Vec API provides a safe truncate/pop_back, consider using it here. If not, the current approach is acceptable and functionally safe given downstream code constructs the peaks Array explicitly.
Can you confirm whether starknet::storage::Vec exposes truncate or pop_back in your current Cairo/StarkNet version? If so, I can propose a concrete diff to replace the zero-fill loop.
contracts/L2/src/dao/Timelock.cairo (1)
126-134: Switch to storage Vec::push is correct; consider bounding calldata length.The
.push(element)change is the right API on newer Starknet storage Vecs. As a defensive measure, consider enforcing a max calldata length to avoid excessive storage writes in one transaction.Example guard near the top of
queue_action:const MAX_CALLDATA_LEN: usize = 1024; // tune per your use-case let calldata_length = calldata.len(); assert!(calldata_length <= MAX_CALLDATA_LEN, "Calldata too large");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
contracts/L2/src/core/ZeroXBridgeL2.cairo(6 hunks)contracts/L2/src/dao/Timelock.cairo(7 hunks)
🔇 Additional comments (9)
contracts/L2/src/core/ZeroXBridgeL2.cairo (5)
3-5: Imports updated for MMR and Array/Hash traits look correct.These align with newer Cairo/StarkNet libraries and cover the traits used below (Poseidon hashing, arrays).
8-17: Interface and OZ/Upgrades imports align with updated toolchain.The dispatcher/component imports match usage below; no API mismatches spotted.
21-27: Storage and env imports are consistent with the Vec push/write pointer usage.Including MutableVecTrait and StoragePointer*Access is necessary for the later .at(...).write(...) and .push(...) calls. Looks good.
400-405: get_last_peaks readback looks good.Iterating storage Vec and building a transient Array via .append is correct and matches downstream usage.
313-313: Semantics unchanged; minor clarity improvement.Returning PRECISION directly (instead of 1 * PRECISION) improves readability with no behavioral change.
contracts/L2/src/dao/Timelock.cairo (4)
1-4: Imports modernization and consolidation LGTM.Switching to core::{array, hash, integer, poseidon} and consolidating starknet imports aligns with newer Cairo/Starknet APIs.
poseidon_hash_spanis appropriately used later for hashing arrays.Also applies to: 6-8
30-31: Correctly importing MutableVecTrait and super::*.
MutableVecTraitis required for.push()on storage Vec;use super::*keeps the dispatcher types (IExecutorDispatcher) in scope within the module. Good update.
90-92: Enum variants converted to unit-like; double-check storage/serde compatibility.Changing from tuple-like empty variants to unit variants should preserve discriminants and thus on-chain encoding. If this contract upgrades in place against existing state, verify no storage decoding issues for
ActionStatus.Would you like me to generate a quick script to scan for any migrations or cross-contract serialization dependencies on
ActionStatus?
170-175: No functional change; array reconstruction remains correct.Loop still reconstructs calldata into memory for the executor call. Looks good.
3 participants
I have update the Starknet foundry, the Starknet and Scarb version and also adjusted the L2 contract to compile without any warning nor error.
Closes #109
Summary by CodeRabbit