Create a test to replicate this issue and fix the bug in the proposal…#116
Create a test to replicate this issue and fix the bug in the proposal…#116bighumus wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe changes introduce a new utility function, Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI (React Hook)
participant Utils as Utils (calculateQuorumFromContracts)
participant Gov as EvsdGovernor Contract
participant Tok as EvsdToken Contract
UI->>Utils: calculateQuorumFromContracts(governor, token)
Utils->>Tok: totalSupply()
Utils->>Gov: quorumNumerator()
Utils->>Gov: quorumDenominator()
Utils-->>UI: quorum (bigint)
sequenceDiagram
participant ProposalReader as BlockchainProposalReader
participant Utils as Utils (calculateQuorumFromContracts)
participant Gov as EvsdGovernor Contract
participant Tok as EvsdToken Contract
participant Vote as getVoteResultForItem
ProposalReader->>Utils: calculateQuorumFromContracts(governor, token)
Utils->>Tok: totalSupply()
Utils->>Gov: quorumNumerator()
Utils->>Gov: quorumDenominator()
Utils-->>ProposalReader: quorum (bigint)
ProposalReader->>Vote: getVoteResultForItem(item, quorum)
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! ✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
hooks/use-quorum.tsx(2 hunks)lib/proposal-services/blockchain/blockchain-proposal-reader.ts(2 hunks)lib/utils.ts(2 hunks)test/blockchain-proposal-service/membership-accept.test.ts(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
test/blockchain-proposal-service/membership-accept.test.ts (4)
lib/proposal-services/blockchain/blockchain-proposal-service.ts (1)
BlockchainProposalService(22-96)types/proposal.ts (3)
User(1-4)UIAddVoterVotableItem(15-18)UIProposal(26-31)lib/utils.ts (1)
calculateQuorumFromContracts(102-111)test/utils.ts (1)
fastForwardTime(165-173)
lib/proposal-services/blockchain/blockchain-proposal-reader.ts (1)
lib/utils.ts (2)
calculateQuorumFromContracts(102-111)getVoteResultForItem(91-96)
hooks/use-quorum.tsx (1)
lib/utils.ts (1)
calculateQuorumFromContracts(102-111)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (22.x)
- GitHub Check: build (22.x)
🔇 Additional comments (9)
lib/utils.ts (3)
16-16: LGTM! Import statement correctly updated.The import statement appropriately includes both contract types needed for the new utility function.
98-100: LGTM! Correct implementation of ceiling division for bigint.The ceiling division implementation using
(a + b - 1n) / bis mathematically correct and follows the standard formula for integer ceiling division.
102-111: LGTM! Well-designed utility function for centralizing quorum calculation.The function correctly:
- Fetches total supply from the token contract
- Retrieves quorum parameters from the governor contract
- Applies ceiling division to compute the final quorum
- Returns the result as a bigint for precision
This centralization will improve maintainability and consistency across the codebase.
lib/proposal-services/blockchain/blockchain-proposal-reader.ts (2)
15-15: LGTM! Import statement correctly includes the new utility function.
83-87: BigInt→Number conversion is safe for expected quorum valuesI searched the codebase for any large hard-coded token supplies or quorum settings and only found small, test-scoped numeric values (e.g. initial voter counts, quorums of 5, etc.). In production the token supply is minted 1:1 to voter addresses, so it will always stay well below Number.MAX_SAFE_INTEGER.
No further changes needed.
hooks/use-quorum.tsx (1)
3-3: LGTM! Import statement correctly includes the utility function.test/blockchain-proposal-service/membership-accept.test.ts (3)
7-9: LGTM! Import statements correctly updated for the new test.The imports properly include the necessary types and utility function for the enhanced test coverage.
17-18: LGTM! Contract instance variables properly declared and initialized.The addition of
evsdTokenandevsdGovernorvariables provides the necessary contract instances for the new quorum calculation test.Also applies to: 31-32
138-170: Excellent test case for quorum threshold edge case.This test properly verifies the critical scenario where:
- All votes are in favor (unanimous)
- Total votes are just below the quorum threshold
- The proposal should correctly fail due to insufficient quorum
The test uses the same
calculateQuorumFromContractsutility as the production code, ensuring consistency and proper integration testing.
| const quorum = calculateQuorumFromContracts(governor, token); | ||
| try { | ||
| setQuorum(Number(quorum)); | ||
| } catch (error) { |
There was a problem hiding this comment.
Critical bug: Missing await for async function call.
The calculateQuorumFromContracts function is async but is not being awaited. This will cause quorum to be a Promise object instead of a bigint, and Number(quorum) will result in NaN.
Apply this fix:
- const quorum = calculateQuorumFromContracts(governor, token);
+ const quorum = await calculateQuorumFromContracts(governor, token);📝 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.
| const quorum = calculateQuorumFromContracts(governor, token); | |
| try { | |
| setQuorum(Number(quorum)); | |
| } catch (error) { | |
| const quorum = await calculateQuorumFromContracts(governor, token); | |
| try { | |
| setQuorum(Number(quorum)); | |
| } catch (error) { |
🤖 Prompt for AI Agents
In hooks/use-quorum.tsx around lines 18 to 21, the call to the async function
calculateQuorumFromContracts is missing an await, causing quorum to be a Promise
instead of the expected bigint. Fix this by adding the await keyword before
calculateQuorumFromContracts to properly wait for the Promise to resolve before
converting it to a Number and setting the state.
1 participant
Implement a test to replicate the issue and fix the bug in the proposal reader
Fixes #115
Summary by CodeRabbit