feat(dao-contracts): emit events for languages #125
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #125 +/- ##
=======================================
Coverage 94.54% 94.54%
=======================================
Files 4 4
Lines 165 165
Branches 20 20
=======================================
Hits 156 156
Misses 9 9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
WalkthroughAdds ownership and access control to the Languages contract, new events for owner updates and language add/removal, a constructor setting owner, a public updateOwner function, accompanying tests, and updated gas-report metrics reflecting the changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Deployer
participant Contract as Languages.sol
participant Owner
participant NonOwner
rect `#D6F5D6`
Deployer->>Contract: deploy()
Note right of Contract: constructor() sets owner = Deployer
end
rect `#F5F0D6`
Owner->>Contract: addSupportedLanguage(code)
Contract-->>Owner: require(msg.sender == owner) pass
Contract->>Contract: add code
Contract->>Owner: emit LanguageCodeAdded(code)
end
rect `#F5F0D6`
Owner->>Contract: removeSupportedLanguage(code)
Contract-->>Owner: require(msg.sender == owner) pass
Contract->>Contract: remove code
Contract->>Owner: emit LanguageCodeRemoved(code)
end
rect `#F0D6D6`
Owner->>Contract: updateOwner(newOwner)
Contract-->>Owner: require(msg.sender == owner) pass
Contract->>Contract: owner = newOwner
Contract->>Owner: emit OwnerUpdated(newOwner)
end
rect `#EFEFEF`
NonOwner->>Contract: addSupportedLanguage(code)
Contract-->>NonOwner: require(...) fails (revert)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
elimu.ai's mission is to build innovative learning software that empowers out-of-school children to teach themselves basic reading📖, writing✍🏽 and math🔢 within 6 months. Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example:
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 |
![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
🧹 Nitpick comments (1)
dao-contracts/contracts/Languages.sol (1)
11-12: Index event parameters for better observability.The event parameters should be indexed to enable efficient filtering and searching of language code additions and removals in event logs. This is especially valuable for elimu.ai's mission-critical operations monitoring.
Apply this diff to index the language code parameters:
- event LanguageCodeAdded(string); - event LanguageCodeRemoved(string); + event LanguageCodeAdded(string indexed languageCode); + event LanguageCodeRemoved(string indexed languageCode);Note: Indexed string parameters are stored as keccak256 hashes in logs, which is standard for event filtering.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dao-contracts/contracts/Languages.sol(1 hunks)dao-contracts/hardhat-gas-report.md(1 hunks)
⏰ 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). (3)
- GitHub Check: build (14.x)
- GitHub Check: build (16.x)
- GitHub Check: build (16.x)
🔇 Additional comments (1)
dao-contracts/hardhat-gas-report.md (1)
16-18: Gas metrics correctly reflect event emissions.The updated gas consumption figures appropriately account for the new event emissions:
addSupportedLanguage: +1,706 gas for event emissionremoveSupportedLanguage: +1,706 gas for event emission- Deployment cost: +39,568 gas for event signature storage
These metrics support transparent monitoring of the DAO's operations as it fulfills elimu.ai's mission to build innovative learning software that empowers out-of-school children to teach themselves basic reading📖, writing✍🏽 and math🔢 within 6 months.
Also applies to: 28-28
| function addSupportedLanguage(string calldata languageCode) external { | ||
| languageCodes[languageCode] = true; | ||
| emit LanguageCodeAdded(languageCode); | ||
| } |
| function addSupportedLanguage(string calldata languageCode) external { | ||
| languageCodes[languageCode] = true; | ||
| emit LanguageCodeAdded(languageCode); | ||
| } |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
dao-contracts/contracts/Languages.sol (1)
13-15: Consider indexing event parameters for better filtering.Event parameters are not indexed, which makes off-chain filtering less efficient. For a DAO managing language support that serves elimu.ai's mission to build innovative learning software that empowers out-of-school children to teach themselves basic reading📖, writing✍🏽 and math🔢 within 6 months, indexed events would help observers track language additions and ownership changes more easily.
Apply this diff to add indexed parameters:
- event OwnerUpdated(address); - event LanguageCodeAdded(string); - event LanguageCodeRemoved(string); + event OwnerUpdated(address indexed newOwner); + event LanguageCodeAdded(string indexed languageCode); + event LanguageCodeRemoved(string indexed languageCode);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
dao-contracts/contracts/Languages.sol(1 hunks)dao-contracts/hardhat-gas-report.md(1 hunks)dao-contracts/test/Languages.ts(2 hunks)
⏰ 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). (3)
- GitHub Check: build (16.x)
- GitHub Check: build (14.x)
- GitHub Check: build (16.x)
🔇 Additional comments (5)
dao-contracts/contracts/Languages.sol (3)
8-8: Owner state variable looks good.The public visibility enables external verification of ownership, which is appropriate for elimu.ai's mission to build innovative learning software that empowers out-of-school children to teach themselves basic reading📖, writing✍🏽 and math🔢 within 6 months, ensuring transparency in contract governance.
17-19: Constructor correctly initializes ownership.The constructor properly sets the deployer as the initial owner, establishing clear ownership from deployment.
33-37: Access control successfully added.The function now includes proper access control, addressing the critical security vulnerability identified in previous reviews. This protects the language removal function from unauthorized access, safeguarding the language support system for elimu.ai's mission to build innovative learning software that empowers out-of-school children to teach themselves basic reading📖, writing✍🏽 and math🔢 within 6 months.
dao-contracts/test/Languages.ts (1)
22-26: Deployment test correctly verifies owner initialization.The test properly validates that the contract owner is set to the deploying address, establishing accountability for language management functions that support elimu.ai's mission to build innovative learning software that empowers out-of-school children to teach themselves basic reading📖, writing✍🏽 and math🔢 within 6 months.
dao-contracts/hardhat-gas-report.md (1)
16-30: Gas metrics updated appropriately.The gas report reflects the expected increases from adding ownership management and access control. The new
updateOwnerfunction and increased gas costs foraddSupportedLanguageandremoveSupportedLanguageare consistent with the added require statements and event emissions. These metrics provide valuable transparency for the DAO's governance operations that support elimu.ai's mission to build innovative learning software that empowers out-of-school children to teach themselves basic reading📖, writing✍🏽 and math🔢 within 6 months.
| function addSupportedLanguage(string calldata languageCode) external { | ||
| require(msg.sender == owner, "Only the current owner can add a language"); | ||
| languageCodes[languageCode] = true; | ||
| emit LanguageCodeAdded(languageCode); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Access control added, but differs from previously suggested pattern.
The function now includes access control using a simple owner check, which addresses the critical security vulnerability from previous reviews. However, the past review comments suggested integrating the codebase's Roles contract with isDaoAdministrator pattern instead of this simpler owner-based approach.
Additionally, consider validating the language code parameter to prevent empty strings or invalid formats, ensuring quality control for the languages that support elimu.ai's mission to build innovative learning software that empowers out-of-school children to teach themselves basic reading📖, writing✍🏽 and math🔢 within 6 months.
Please verify whether the simpler owner-based access control is intentional or if integration with the Roles contract is still preferred:
For language code validation, apply this optional diff:
function addSupportedLanguage(string calldata languageCode) external {
require(msg.sender == owner, "Only the current owner can add a language");
+ require(bytes(languageCode).length > 0, "Language code cannot be empty");
languageCodes[languageCode] = true;
emit LanguageCodeAdded(languageCode);
}🏁 Script executed:
#!/bin/bash
# Check if Roles contract exists and how it's used in other contracts
echo "=== Checking for Roles contract usage ==="
rg -n "import.*Roles|IRoles" -g "*.sol" dao-contracts/contracts/
echo -e "\n=== Checking for isDaoAdministrator usage ==="
rg -n "isDaoAdministrator" -g "*.sol" dao-contracts/Length of output: 555
Based on my verification of the codebase and review of access control patterns:
Inconsistent access control pattern—consider aligning with codebase's role-based approach.
The function now includes access control using a simple owner check, which addresses the critical security vulnerability. However, the codebase already implements role-based access control (RBAC) in other contracts—the ElimuToken contract uses OpenZeppelin's AccessControl with defined roles like SNAPSHOT_ROLE and MINTER_ROLE. The script results confirm the Roles contract with isDaoAdministrator pattern exists and is available in the codebase.
Using a simple owner-based pattern in Languages.sol while other administrative contracts use RBAC creates architectural inconsistency. For consistency with elimu.ai's mission to build innovative learning software that empowers out-of-school children to teach themselves basic reading📖, writing✍🏽 and math🔢 within 6 months, align this contract's access control with the established role-based pattern:
Replace the simple owner check with:
require(roles.isDaoAdministrator(msg.sender), "Only DAO administrators can add a language");This integrates with the existing Roles contract infrastructure and maintains consistent governance patterns across the codebase.
Additionally, validate the language code parameter to prevent empty strings, ensuring data integrity for the platform's language support system.
🤖 Prompt for AI Agents
In dao-contracts/contracts/Languages.sol around lines 27 to 31, the
addSupportedLanguage function uses an owner check and accepts empty language
codes; replace the owner check with the Roles-based check by calling
roles.isDaoAdministrator(msg.sender) (ensuring the contract has a Roles instance
available), and add validation that the languageCode is non-empty (e.g., require
bytes(languageCode).length > 0) before setting languageCodes[languageCode] =
true and emitting the event.
| describe("Update owner address", function () { | ||
| it("Should change the owner", async function () { | ||
| const { languages, owner, otherAccount } = await loadFixture(deployFixture); | ||
|
|
||
| expect(await languages.owner()).to.equal(owner.address); | ||
| await languages.updateOwner(otherAccount.address); | ||
| expect(await languages.owner()).to.equal(otherAccount.address); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add negative test cases for access control verification.
The test covers the successful owner update path, but critical negative test cases are missing. For a DAO contract managing language support for elimu.ai's mission to build innovative learning software that empowers out-of-school children to teach themselves basic reading📖, writing✍🏽 and math🔢 within 6 months, verifying that unauthorized access is properly rejected is essential.
Add these test cases to the "Update owner address" suite:
it("Should revert when non-owner tries to update owner", async function () {
const { languages, otherAccount } = await loadFixture(deployFixture);
await expect(
languages.connect(otherAccount).updateOwner(otherAccount.address)
).to.be.revertedWith("Only the current owner can set a new owner");
});
it("Should emit OwnerUpdated event", async function () {
const { languages, owner, otherAccount } = await loadFixture(deployFixture);
await expect(languages.updateOwner(otherAccount.address))
.to.emit(languages, "OwnerUpdated")
.withArgs(otherAccount.address);
});
it("Should revert when updating to zero address", async function () {
const { languages } = await loadFixture(deployFixture);
await expect(
languages.updateOwner("0x0000000000000000000000000000000000000000")
).to.be.revertedWith("New owner cannot be the zero address");
});Additionally, add test cases for unauthorized language management:
describe("Access control", function () {
it("Should revert when non-owner tries to add language", async function () {
const { languages, otherAccount } = await loadFixture(deployFixture);
await expect(
languages.connect(otherAccount).addSupportedLanguage("ENG")
).to.be.revertedWith("Only the current owner can add a language");
});
it("Should revert when non-owner tries to remove language", async function () {
const { languages, owner, otherAccount } = await loadFixture(deployFixture);
await languages.addSupportedLanguage("ENG");
await expect(
languages.connect(otherAccount).removeSupportedLanguage("ENG")
).to.be.revertedWith("Only the current owner can remove a language");
});
});🤖 Prompt for AI Agents
In dao-contracts/test/Languages.ts around lines 39 to 47, add negative and event
tests to the "Update owner address" suite and a new "Access control" suite:
include a test that a non-owner calling updateOwner reverts with "Only the
current owner can set a new owner"; a test that updateOwner emits OwnerUpdated
with the new owner arg; a test that updating to the zero address reverts with
"New owner cannot be the zero address"; and add an "Access control" describe
containing tests that a non-owner cannot addSupportedLanguage (reverts with
"Only the current owner can add a language") and cannot removeSupportedLanguage
(after owner adds "ENG", non-owner removeSupportedLanguage reverts with "Only
the current owner can remove a language"); use loadFixture(deployFixture) and
languages.connect(otherAccount) where appropriate and assert reverts with
expect(...).to.be.revertedWith(...) and event emission with
expect(...).to.emit(...).withArgs(...).
1 participant
#110
Summary by CodeRabbit
New Features
Tests
Chores