feat: allow crisp to work with constant credits#1263
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
📝 WalkthroughWalkthroughAdds CreditMode (CONSTANT vs CUSTOM) and credits across contracts, SDK, client, and server; propagates new fields through models and repo; introduces a constant-balance token-holder discovery path and adjusts snapshot block handling in the indexer/etherscan logic. Changes
Sequence DiagramsequenceDiagram
participant Client
participant CRISPContract as CRISP Contract
participant ServerIndexer as Server Indexer
participant EtherscanClient
participant Repository
participant Blockchain
Client->>CRISPContract: startRound(customParams with creditMode, credits)
CRISPContract->>ServerIndexer: emit validate event (includes customParams)
ServerIndexer->>ServerIndexer: decode customParams -> token, threshold, num_options, credit_mode, credits
alt credit_mode == CONSTANT
ServerIndexer->>EtherscanClient: get_token_holders_with_constant_balance(token, snapshotBlock-1, credits)
EtherscanClient->>EtherscanClient: analyze transfer & delegation logs
EtherscanClient-->>ServerIndexer: TokenHolders (assigned constant balance)
else credit_mode == CUSTOM
ServerIndexer->>EtherscanClient: get_token_holders_with_voting_power(token, snapshotBlock-1, threshold)
EtherscanClient->>Blockchain: query voting power per holder
EtherscanClient-->>ServerIndexer: TokenHolders (variable balances)
end
ServerIndexer->>Repository: initialize_round(customParams, requester)
Repository-->>ServerIndexer: persisted E3 state
ServerIndexer->>ServerIndexer: compute merkle root
ServerIndexer->>Blockchain: set merkle root on-chain
Blockchain-->>ServerIndexer: tx confirmation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
ctrlc03
force-pushed
the
feat/constant-credits-crisp
branch
from
df4f118 to
9fbd138
Compare
ctrlc03
force-pushed
the
feat/constant-credits-crisp
branch
from
522caf7 to
387cec4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
examples/CRISP/packages/crisp-contracts/contracts/Mocks/MockEnclave.sol (1)
20-40:⚠️ Potential issue | 🟠 MajorFix
validatecall to pass the new 5-fieldcustomParamsencoding.Line 31 encodes five fields:
abi.encode(address(0), nextE3Id, 2, 0, 0), but line 39 passes only three:abi.encode(address(0), nextE3Id, 2). CRISPProgram'svalidatemethod (line 133) decodes all five fields—address, uint256, uint256, CreditMode, and uint256—so this mismatch will cause a revert onabi.decode.Proposed fix
- IE3Program(program).validate(nextE3Id, 0, bytes(""), bytes(""), abi.encode(address(0), nextE3Id, 2)); + IE3Program(program).validate(nextE3Id, 0, bytes(""), bytes(""), abi.encode(address(0), nextE3Id, 2, 0, 0));examples/CRISP/server/src/server/models.rs (1)
142-252:⚠️ Potential issue | 🟠 MajorBack-compat risk: new non-optional fields will break deserialization of existing persisted records.
The addition of
credit_modeandcreditsfields toCustomParams,E3StateLite, andE3Crispcreates a breaking change. Data is serialized to JSON via serde_json and persisted in a sled database. Any existing stored records lacking these fields will fail to deserialize whenget_crisp()or related methods attempt to load them, causing runtime panics.Add serde defaults to handle records created before these fields existed:
Suggested fix
+fn default_credit_mode() -> CreditMode { + CreditMode::Custom +} + #[derive(Debug, Deserialize, Serialize)] pub struct CustomParams { pub token_address: String, pub balance_threshold: String, pub num_options: String, + #[serde(default = "default_credit_mode")] pub credit_mode: CreditMode, + #[serde(default)] pub credits: Option<String>, } #[derive(Debug, Deserialize, Serialize)] pub struct E3StateLite { // ... + #[serde(default = "default_credit_mode")] pub credit_mode: CreditMode, + #[serde(default)] pub credits: Option<String>, } #[derive(Debug, Deserialize, Serialize)] pub struct E3Crisp { // ... + #[serde(default = "default_credit_mode")] pub credit_mode: CreditMode, + #[serde(default)] pub credits: Option<String>, }examples/CRISP/server/src/server/repo.rs (1)
168-188:⚠️ Potential issue | 🟡 MinorAdd validation for
credit_mode/creditsconsistency ininitialize_round.While
CustomParamsis currently constructed with a match statement inindexer.rsthat ensuresCreditMode::Constantalways hascreditsset,initialize_roundacceptsCustomParamswithout explicit validation. To make the contract explicit and prevent issues if new code paths createCustomParamsin the future, add a guard to rejectCreditMode::Constantwithcredits = Nonebefore persisting.🔧 Suggested guard
+ if matches!(&custom_params.credit_mode, crate::server::models::CreditMode::Constant) + && custom_params.credits.is_none() + { + return Err(eyre::eyre!( + "credits must be provided when credit_mode is Constant" + )); + } self.set_crisp(E3Crisp { has_voted: vec![], start_time: 0u64, status: "Requested".to_string(),
🤖 Fix all issues with AI agents
In `@examples/CRISP/packages/crisp-sdk/src/types.ts`:
- Around line 226-234: The SDK's CreditMode enum in types.ts uses string values
('0'/'1') which won't strictly equal the server's numeric JSON discriminants;
update the enum CreditMode to use numeric values (0 and 1) so deserialized JSON
numbers match (e.g., export enum CreditMode { CONSTANT = 0, CUSTOM = 1 }), and
then audit any strict comparisons or switch statements that reference CreditMode
to ensure they still work with numbers.
In `@examples/CRISP/server/src/server/token_holders/etherscan.rs`:
- Around line 582-637: In get_token_holders_with_constant_balance, the
constant-balance branch currently admits any potential voter with nonzero
balance or delegation without consulting the configured balance_threshold;
either enforce the threshold by filtering potential_voters using the configured
balance_threshold (e.g., require v.token_balance >= balance_threshold before
mapping to TokenHolder) or explicitly validate early that balance_threshold ==
U256::ZERO and return an error if not; update the filter that produces
token_holders in get_token_holders_with_constant_balance to reference the
balance_threshold (or add the validation) so eligibility respects the configured
threshold.
🧹 Nitpick comments (4)
examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol (1)
132-139: Add a guard for invalidcreditModevalues.
abi.decodewill accept any numeric value for the enum; a quick bounds check prevents invalid modes from being persisted on-chain.Suggested guard
+ error InvalidCreditMode(); ... - (, , uint256 numOptions, CreditMode creditMode, ) = abi.decode(customParams, (address, uint256, uint256, CreditMode, uint256)); + (, , uint256 numOptions, CreditMode creditMode, ) = abi.decode(customParams, (address, uint256, uint256, CreditMode, uint256)); + if (uint8(creditMode) > uint8(CreditMode.CUSTOM)) revert InvalidCreditMode();examples/CRISP/server/src/server/models.rs (1)
277-295: Define a stable serialization format forCreditModeacross layers.Right now the Rust enum will serialize as variant names by default, while the SDK uses
'0'/'1'and Solidity uses numeric enum values. Please confirm the intended wire format and add explicit serde mapping (into/try_from u64or renamed strings) to avoid cross‑layer mismatches.examples/CRISP/server/src/server/indexer.rs (1)
124-146: Guard againstrequestBlockunderflow.
requestBlock - 1will wrap if the value is 0 (in release builds), producing a huge block number and expensive log queries. Asaturating_sub(1)or explicit check is safer.Safer subtraction
- event.e3.requestBlock.to::<u64>() - 1u64, + event.e3.requestBlock.to::<u64>().saturating_sub(1),examples/CRISP/server/src/server/token_holders/etherscan.rs (1)
438-455: Documentget_past_votesparameter semantics for clarity.The function passes
block_numberdirectly togetPastVoteswithout adjustment. Current callers (e.g.,indexer.rs:145) correctly handle this by passingsnapshot_block - 1, but the function lacks a docstring explaining this requirement. Consider documenting that the parameter should be a historical block (typicallysnapshot_block - 1for snapshot voting), or rename the parameter to make semantics explicit and prevent future mistakes.
ctrlc03
force-pushed
the
feat/constant-credits-crisp
branch
from
9465cf3 to
71e4bdd
Compare
cedoor
force-pushed
the
feat/constant-credits-crisp
branch
from
71e4bdd to
f9db40a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol`:
- Around line 133-139: The inline comment contains a typo: replace "credit more"
with "credit mode" in the comment above the assignment to
e3Data[e3Id].creditMode; verify the comment near the assignments to
e3Data[e3Id].numOptions and e3Data[e3Id].creditMode correctly reads "we want to
save the credit mode so it can be verified on chain by everyone" and keep the
surrounding logic (decoding customParams, the numOptions check that may revert
InvalidNumOptions()) unchanged.
In `@examples/CRISP/server/src/server/models.rs`:
- Around line 276-296: The enum CreditMode is currently derived with
Serialize/Deserialize which emits variant names ("Constant"/"Custom") instead of
numeric discriminants expected by the TypeScript SDK; update the enum to use
serde_repr's Serialize_repr and Deserialize_repr and add a repr(u64) (or
repr(u8) if smaller) so JSON serializes to 0/1, and also derive Clone and Copy
for ergonomics; keep the existing TryFrom<u64> impl (CreditMode and TryFrom)
unchanged so conversion logic remains intact. Ensure serde_repr is added to
Cargo.toml and the enum declaration uses #[derive(Debug, PartialEq, Clone, Copy,
Serialize_repr, Deserialize_repr)] and #[repr(u64)] (or repr(u8)) to serialize
numeric discriminants.
🧹 Nitpick comments (2)
examples/CRISP/server/src/server/token_holders/etherscan.rs (1)
582-645: Consider extracting shared discovery steps to reduce duplication.Steps 1–4 (deployment block lookup, transfer log fetch, delegation log fetch, potential voter identification) are identical between
get_token_holders_with_voting_powerandget_token_holders_with_constant_balance. Extracting these into a shared helper would reduce ~40 lines of duplication and make both paths easier to maintain.Sketch
// Private helper async fn discover_potential_voters( &self, token_address: Address, snapshot_block: u64, ) -> Result<Vec<PotentialVoter>> { let start_block = self.get_deployment_block(&token_address.to_string()).await?; let transfer_logs = self.get_transfer_logs(&token_address.to_string(), start_block, snapshot_block).await?; let delegation_logs = self.get_delegate_votes_changed_logs(&token_address.to_string(), start_block, snapshot_block).await?; Ok(self.get_potential_voters(&transfer_logs, &delegation_logs)) }Then both public methods call
discover_potential_votersand only diverge in step 5.examples/CRISP/server/src/server/indexer.rs (1)
89-97:credits_clonecan be eliminated by referencingcustom_params.creditsdirectly inside the match.The
credits_clonevariable exists solely becausecreditsis moved intocustom_params. Sincecustom_paramsis still available inside the match, you can borrow or clone from it directly:Proposed simplification
- let credits_clone = credits.clone(); - let custom_params = CustomParams { ... }; ... match custom_params.credit_mode { CreditMode::Constant => { - let credits_str = credits_clone.expect("credits must be set for Constant mode"); + let credits_str = custom_params.credits.clone().expect("credits must be set for Constant mode");This removes the intermediate variable and keeps the intent clearer.
Also applies to: 123-161
- Updated @crisp-e3/sdk to 0.5.10 - Updated @crisp-e3/contracts to 0.5.10 - Updated @crisp-e3/zk-inputs to 0.5.10 - Published to npm
ctrlc03
force-pushed
the
feat/constant-credits-crisp
branch
from
f9db40a to
ccbf0bb
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@examples/CRISP/server/src/cli/commands.rs`:
- Line 104: Fix the typo in the comment that reads "The credit mode is costant
for the CRISP app (everyone gets the same credits)" by changing "costant" to
"constant" in the comment near the credits/credit mode logic in
examples/CRISP/server/src/cli/commands.rs (look for the comment text or the
function handling credit mode/credits to locate it).
In `@examples/CRISP/server/src/server/indexer.rs`:
- Around line 130-138: The subtraction of 1 from
event.e3.requestBlock.to::<u64>() can underflow; update the call sites that pass
the block argument to etherscan_client.get_token_holders_with_constant_balance
(and the equivalent "Custom" branch) to defensively compute the prior block
using saturating_sub(1) or an explicit guard (e.g., let prior_block =
event.e3.requestBlock.to::<u64>().saturating_sub(1); then pass prior_block) so
the code will not panic in debug mode or wrap in release.
🧹 Nitpick comments (3)
examples/CRISP/server/Cargo.toml (1)
57-57: Version pinning style is inconsistent with other dependencies.Most dependencies in this file use exact pinning with
=prefix (e.g.,"=0.7.1","=4.11.0"), butserde_repruses"0.1.20"(SemVer-compatible range). Consider using"=0.1.20"for consistency.Proposed fix
-serde_repr = "0.1.20" +serde_repr = "=0.1.20"examples/CRISP/server/src/server/token_holders/etherscan.rs (1)
582-644: Extract shared discovery logic (steps 1–4) to reduce duplication withget_token_holders_with_voting_power.Steps 1–4 (get deployment block → fetch transfer logs → fetch delegation logs → identify potential voters) are copy-pasted from
get_token_holders_with_voting_power(lines 519–555). Extract a private helper likediscover_potential_voters(token_address, snapshot_block) -> Result<Vec<PotentialVoter>>and call it from both methods.♻️ Sketch of the extracted helper
+ /// Shared discovery steps: deployment block → transfer logs → delegation logs → potential voters + async fn discover_potential_voters( + &self, + token_address: Address, + snapshot_block: u64, + ) -> Result<Vec<PotentialVoter>> { + let start_block = self + .get_deployment_block(&token_address.to_string()) + .await + .context("Failed to get deployment block")?; + log::info!("Token deployed at block: {}", start_block); + + let transfer_logs = self + .get_transfer_logs(&token_address.to_string(), start_block, snapshot_block) + .await + .context("Failed to fetch transfer logs")?; + log::info!("Found {} transfer events", transfer_logs.len()); + + let delegation_logs = self + .get_delegate_votes_changed_logs(&token_address.to_string(), start_block, snapshot_block) + .await + .context("Failed to fetch delegation logs")?; + log::info!("Found {} delegation events", delegation_logs.len()); + + let potential_voters = self.get_potential_voters(&transfer_logs, &delegation_logs); + log::info!("Found {} potential voters", potential_voters.len()); + + Ok(potential_voters) + }Then both public methods simply call
self.discover_potential_voters(...)before diverging at step 5.examples/CRISP/server/src/server/indexer.rs (1)
124-139: RedundantU256 → String → U256round-trip forcredits.
decoded.4is already aU256. Converting it to aString(line 81), cloning it (line 89), then parsing it back toU256(lines 127–128) is unnecessary work in this branch. Consider capturingdecoded.4directly for the Etherscan call and only stringifying it forCustomParams.♻️ Sketch
+ let credits_u256 = decoded.4; // keep the original U256 + let credits = match credit_mode { CreditMode::Constant => { info!("[e3_id={}] Credit mode: Constant", e3_id); Some(decoded.4.to_string()) } CreditMode::Custom => { info!("[e3_id={}] Credit mode: Custom", e3_id); None } }; - - let credits_clone = credits.clone(); // ... later in Constant branch: - let credits_str = credits_clone.expect("credits must be set for Constant mode"); - let credits_u256: alloy_primitives::Uint<256, 4> = U256::from_str_radix(&credits_str, 10) - .map_err(|e| eyre::eyre!("Failed to parse credits: {}", e))?; + // use credits_u256 directly
2 participants
fix #1256
fix #1004
Summary by CodeRabbit
New Features
Chores
API