HUBS-224 crowdsale with individual locking staking periods #170
Conversation
adds condition to faile when duration < 7 days (enfoced by token vesting contract) replaces bakedin locking contract with an external deployment for verifiability Signed-off-by: Stefan Adolf <stefan@molecule.to>
adds staking duration parameters to all staking tests drops old V23 sale rollout script Signed-off-by: Stefan Adolf <stefan@molecule.to>
WalkthroughThis pull request introduces significant enhancements to the crowd sale and token locking mechanisms across multiple contracts and configurations. The primary focus is on integrating a new Changes
Sequence DiagramsequenceDiagram
participant Deploy as Deployment Script
participant TimelockedToken
participant LockingCrowdSale
participant StakedLockingCrowdSale
Deploy->>TimelockedToken: Create Implementation
Deploy->>LockingCrowdSale: Initialize with TimelockedToken
Deploy->>StakedLockingCrowdSale: Initialize with TimelockedToken
StakedLockingCrowdSale->>StakedLockingCrowdSale: Set Stake Vesting Duration
LockingCrowdSale->>LockingCrowdSale: Create/Trust Locking Contracts
Poem
Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
Signed-off-by: Stefan Adolf <stefan@molecule.to>
elmariachi111
force-pushed
the
feature/hubs-224-crowdsale-with-individual-locking-staking-periods
branch
from
def2bed to
8d83cfc
Compare
Signed-off-by: Stefan Adolf <stefan@molecule.to>
Signed-off-by: Stefan Adolf <stefan@molecule.to>
Signed-off-by: Stefan Adolf <stefan@molecule.to>
Signed-off-by: Stefan Adolf <stefan@molecule.to>
Signed-off-by: Stefan Adolf <stefan@molecule.to>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (18)
subgraph/src/lockingCrowdSaleMapping.ts (3)
2-2: Consider renaming the imported BigInt.
Although this is a type from '@graphprotocol/graph-ts', it shadows the global 'BigInt' in TypeScript, which might lead to confusion. Renaming avoids collisions and clarifies the origin.-import { - BigInt, - ... -} from '@graphprotocol/graph-ts' +import { + BigInt as GraphBigInt, + ... +} from '@graphprotocol/graph-ts'🧰 Tools
🪛 Biome (1.9.4)
[error] 2-2: Do not shadow the global "BigInt" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
67-80: Ensure locked token uniqueness is correct and tested.
The code checks if the new locking token differs from the existing one and logs an error if they don't match. Check if any fallback or handle path is needed when a mismatch occurs, especially if an IP might inadvertently reference multiple locking tokens.
81-87: Optional: Distinguish the crowdSale.type assignment for future expansions.
Currently, "STAKED_LOCKING_CROWDSALE" is assigned. If there’s a possibility of different crowd sale subtypes, consider storing them in a more flexible, future-proof manner (e.g. an enum or additional fields).src/crowdsale/LockingCrowdSale.sol (1)
65-70: Consider clarifying the fallback logic with a comment.
When the trustee returns no existing contract, a fresh TimelockedToken is created. A short comment explaining the fallback scenario can improve clarity for future maintainers.src/crowdsale/StakedLockingCrowdSale.sol (1)
127-130: Large vesting durations.
You enforce a minimum of 7 days. Consider also an upper limit if business logic demands a max..env.example (2)
29-29: Standardize comment formattingThe comment style is inconsistent. Consider using a consistent format:
-#iptoken implementation=0x0DCd1Bf9A1b36cE34237eEaFef220932846BCD82 -#timelocked token implementation=0x0B306BF915C4d645ff596e518fAf3F9669b97016 +# IPToken implementation = 0x0DCd1Bf9A1b36cE34237eEaFef220932846BCD82 +# Timelocked token implementation = 0x0B306BF915C4d645ff596e518fAf3F9669b97016Also applies to: 31-31
🧰 Tools
🪛 Gitleaks (8.21.2)
29-29: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
34-36: Consider grouping related addressesConsider grouping the LOCKED_IPTS_ADDRESS with other IPT-related addresses for better organization and readability.
USDC6_ADDRESS=0x3Aa5ebB10DC797CAC828524e59A333d0A371443c WETH_ADDRESS=0x322813Fd9A801c5507c9de605d63CEA4f2CE6c44 PLAIN_CROWDSALE_ADDRESS=0x09635F643e140090A9A8Dcd712eD6285858ceBef -#these are generated when running the fixture scripts -IPTS_ADDRESS=0x8dAF17A20c9DBA35f005b6324F493785D239719d -LOCKED_IPTS_ADDRESS=0x24B3c7704709ed1491473F30393FFc93cFB0FC34 +# IPT-related addresses (generated when running the fixture scripts) +IPTS_ADDRESS=0x8dAF17A20c9DBA35f005b6324F493785D239719d +LOCKED_IPTS_ADDRESS=0x24B3c7704709ed1491473F30393FFc93cFB0FC34Also applies to: 40-40
setupLocal.sh (2)
Line range hint
21-32: Consider adding validation for option combinationsThe script might benefit from validating mutually exclusive options or requiring certain option combinations.
while getopts "fx" opt; do case ${opt} in f) fixtures=true ;; x) + if [ "$fixtures" = false ]; then + echo "Error: -x requires -f option" + exit 1 + fi extrafixtures=true ;; *) echo "Unknown option: $1" exit 1 ;; esac done
Line range hint
66-73: Consider parameterizing the sleep durationThe hardcoded sleep duration of 15/16 seconds might need adjustment based on network conditions. Consider making it configurable.
+# Default sleep duration between operations (in seconds) +SLEEP_DURATION=${SLEEP_DURATION:-15} + if $extrafixtures; then echo "Running extra fixture scripts (crowdsales)." $FSC script/dev/CrowdSale.s.sol:FixtureCrowdSale - echo "Waiting 15 seconds until claiming plain sale..." - sleep 16 + echo "Waiting ${SLEEP_DURATION} seconds until claiming plain sale..." + sleep $((SLEEP_DURATION + 1))script/DeployTokenizer.s.sol (2)
31-33: Improve comment clarity and consider adding eventsThe comment about contract verification could be more descriptive. Also, consider emitting events for important state changes.
- //this allows the default TimelockedToken implementation to be verified on chain explorers + // Deploy TimelockedToken implementation first to enable contract verification on block explorers TimelockedToken timelockedTokenImplementation = new TimelockedToken(); + emit TimelockedTokenImplementationDeployed(address(timelockedTokenImplementation)); StakedLockingCrowdSale stakedLockingCrowdSale = new StakedLockingCrowdSale(timelockedTokenImplementation); + emit StakedLockingCrowdSaleDeployed(address(stakedLockingCrowdSale));Don't forget to add the event declarations at the contract level:
event TimelockedTokenImplementationDeployed(address implementation); event StakedLockingCrowdSaleDeployed(address crowdSale);
40-41: Consider standardizing the console log formatThe console log format for the timelocked token implementation differs from other logs. Consider standardizing it:
- console.log("timelocked token implementation=%s", address(timelockedTokenImplementation)); + console.log("TIMELOCKED_TOKEN_IMPLEMENTATION_ADDRESS=%s", address(timelockedTokenImplementation));script/prod/RolloutV25Sale.sol (2)
15-15: Document multisig addressesThe multisig addresses are hardcoded without clear documentation of their purpose and ownership. Consider adding comments explaining:
- Who controls these multisigs
- Why different addresses are used for mainnet vs other networks
- Whether these have been verified as the correct addresses
Also applies to: 34-34
18-21: Consider emitting events for important state changesImportant contract deployments and ownership transfers should emit events for better traceability. Consider:
- Adding events to the contracts being deployed
- Using OpenZeppelin's
Initializableto prevent double initializationExample implementation:
event CrowdsaleDeployed(address indexed implementation, address indexed proxy, address indexed owner);Also applies to: 40-43
test/CrowdSaleWithNonStandardERC20Test.t.sol (1)
87-87: Verify consistent vesting duration usageThe startSale function calls now include the vesting duration parameter. The values used are:
- 60 days for standard sales
- 7 days for the 2 decimals test case
Consider extracting these durations as constants to maintain consistency and make updates easier.
+ uint256 constant STANDARD_VESTING_DURATION = 60 days; + uint256 constant SHORT_VESTING_DURATION = 7 days;Also applies to: 125-125, 183-183, 245-245, 310-310
test/CrowdSaleLockedStakedTest.t.sol (1)
459-461: Implement the pending test for locking contract reuseThe TODO comment indicates a missing test case for demonstrating how locking contracts are reused.
Would you like me to help implement this test case or create a GitHub issue to track this task?
subgraph/abis/LockingCrowdSale.json (1)
319-435: Consider adding validation for minimum sale durationThe
startSalefunctions should consider adding validation for minimum sale duration to prevent flash sales that could be manipulated.Consider adding a minimum duration check in the implementation to prevent potential market manipulation through extremely short sales.
README.md (2)
53-54: Consider improving the format of implementation addressesThe timelocked token implementation address and the deprecated crowdsale note could be better formatted for consistency with the rest of the document. Consider using the table format or a dedicated section for implementation addresses.
-timelocked token implementation=0xF8F79c1E02387b0Fc9DE0945cD9A2c06F127D851 -~~new staked crowdsale (not used yet HUBS-224) 0x7eeb7113f90893fb95c6666e3930235850f2bc6A)~~ +| Contract | Address | Status | +| -------- | ------- | ------ | +| Timelocked Token Implementation | 0xF8F79c1E02387b0Fc9DE0945cD9A2c06F127D851 | Active | +| Staked Crowdsale | 0x7eeb7113f90893fb95c6666e3930235850f2bc6A | Deprecated (HUBS-224) |🧰 Tools
🪛 Gitleaks (8.21.2)
53-53: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
212-214: Fix typo in verification instructionsThe word "crowdsale" is repeated in the instruction header.
-Verifying a the staked crowdsale crowdsale +Verifying the staked crowdsale🧰 Tools
🪛 LanguageTool
[grammar] ~212-~212: Two determiners in a row. Choose either “a” or “the”.
Context: ....sol:StakedLockingCrowdSaleVerifying a the staked crowdsale crowdsaleforge verif...(DT_DT)
[duplication] ~212-~212: Possible typo: you repeated a word
Context: ...ckingCrowdSaleVerifying a the staked crowdsale crowdsaleforge verify-contract --chain-id=11155...(ENGLISH_WORD_REPEAT_RULE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
.env.example(1 hunks)README.md(2 hunks)script/DeployTokenizer.s.sol(2 hunks)script/dev/CrowdSale.s.sol(2 hunks)script/dev/SignTermsMessage.s.sol(1 hunks)script/dev/Tokenizer.s.sol(1 hunks)script/prod/RolloutV23Sale.sol(0 hunks)script/prod/RolloutV25Sale.sol(1 hunks)setupLocal.sh(1 hunks)src/crowdsale/LockingCrowdSale.sol(3 hunks)src/crowdsale/StakedLockingCrowdSale.sol(7 hunks)subgraph/abis/IPNFT.json(1 hunks)subgraph/abis/LockingCrowdSale.json(1 hunks)subgraph/abis/StakedLockingCrowdSale.json(5 hunks)subgraph/makeAbis.sh(1 hunks)subgraph/networks.json(3 hunks)subgraph/schema.graphql(1 hunks)subgraph/src/lockingCrowdSaleMapping.ts(1 hunks)subgraph/subgraph.yaml(2 hunks)test/CrowdSaleLockedStakedTest.t.sol(9 hunks)test/CrowdSaleLockedTest.t.sol(1 hunks)test/CrowdSalePermissioned.t.sol(2 hunks)test/CrowdSaleWithNonStandardERC20Test.t.sol(6 hunks)
💤 Files with no reviewable changes (1)
- script/prod/RolloutV23Sale.sol
✅ Files skipped from review due to trivial changes (1)
- script/dev/Tokenizer.s.sol
🧰 Additional context used
🪛 Gitleaks (8.21.2)
README.md
53-53: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
.env.example
29-29: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
31-31: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 LanguageTool
README.md
[grammar] ~212-~212: Two determiners in a row. Choose either “a” or “the”.
Context: ....sol:StakedLockingCrowdSale Verifying a the staked crowdsale crowdsaleforge verif...
(DT_DT)
[duplication] ~212-~212: Possible typo: you repeated a word
Context: ...ckingCrowdSale Verifying a the staked crowdsale crowdsaleforge verify-contract --chain-id=11155...
(ENGLISH_WORD_REPEAT_RULE)
🪛 Biome (1.9.4)
subgraph/src/lockingCrowdSaleMapping.ts
[error] 2-2: Do not shadow the global "BigInt" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🔇 Additional comments (34)
subgraph/src/lockingCrowdSaleMapping.ts (1)
32-87: Verify error handling when loading the CrowdSale/ IPT entities.
The logic already logs an error if the entity doesn't exist, then returns. Confirm that upstream code or tests ensure that the missing entities do not leave the subgraph in an unexpected state.
✅ Verification successful
Let me check the error handling patterns in these files to ensure consistency.
Error handling for CrowdSale/IPT entities is consistent and safe
The error handling patterns across the codebase are consistent and properly handle missing entities:
- All CrowdSale.load() calls check for null and log appropriate errors before returning
- All IPT.load() calls (except in common.ts which has a valid different use case) check for null, log errors, and return
- The error handling in lockingCrowdSaleMapping.ts follows the same pattern as other files
- Early returns prevent any further processing that could leave the subgraph in an inconsistent state
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Use ast-grep to locate entity loading in the subgraph code to ensure consistent error handling
ast-grep --pattern $'CrowdSale.load($_)'
ast-grep --pattern $'IPT.load($_)'
Length of output: 1608
Script:
#!/bin/bash
# Check error handling patterns in files that load CrowdSale and IPT entities
rg "CrowdSale.load|IPT.load" -A 5 subgraph/src/
Length of output: 6617
src/crowdsale/LockingCrowdSale.sol (6)
15-17: Contract naming clarity.
The interface name, ITrustedLockingContracts, accurately conveys that it provides externally trusted references to TimelockedToken instances. This is straightforward, good naming.
41-43: Constructor injection approach looks solid.
Injecting the TimelockedToken implementation address via the constructor is a robust approach that aligns well with proxy-based patterns.
50-52: Check the usage scope of trustLockingContractSource.
This function sets a trustee for locking contracts. For clarity and security, confirm that only the owner may call it and that calling it multiple times is safe.
Line range hint 84-88: Locking duration upper bound check is valid and conventional.
The revert condition for > 366 days is explicit and straightforward, preventing unreasonably long locks.
138-138: Clone usage is correct; confirm initialization is safe from reentrancy.
The code clones and initializes a TimelockedToken. Consider verifying that reentrancy cannot occur during initialization.
24-35: Consider verifying correctness of public variables.
The new public mappings and references allow external reading of lockingContracts. Confirm that read access to these mappings does not expose sensitive data or compromise security.
✅ Verification successful
Based on the search results, I can now generate the final response:
Public mappings for locking contracts are safely implemented
The lockingContracts mapping and lockingContractTrustee are used appropriately with proper access controls:
- Locking contracts can only be created through
createOrReturnTimelockContract()which enforces proper initialization - Writing to
lockingContractsonly happens in controlled contexts with owner-only access for trustee configuration - The mappings only expose read access to contract addresses, which is necessary for verification and integration
- Test coverage confirms expected behavior for token locking, vesting and release mechanics
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching for usage of lockingContracts and lockingContractTrustee references to ensure no unintended writes or missed permission checks
rg lockingContracts -A 10
rg lockingContractTrustee -A 10
Length of output: 31917
src/crowdsale/StakedLockingCrowdSale.sol (4)
41-42: Mapping for staking durations is aligned with the new schema.
The new mapping 'salesStakeVestingDuration' is consistent with the broader locking logic.
57-58: Constructor chaining is appropriate.
Passing the _timelockedTokenImplementation to LockingCrowdSale ensures consistent TimelockedToken usage across both contracts.
88-90: Ensure lockingDuration and stakesVestingDuration validations remain consistent.
Both parameters have separate checks (e.g., stake vesting must be ≥ 7 days, while locking must be ≤ 366 days). Confirm that test cases cover boundary scenarios (e.g. exactly 7 days, exactly 366 days).
Also applies to: 98-99
Line range hint 196-222: Double-check vesting logic in the claim function.
Tokens are immediately transferred to the user if the sale is closed + vesting duration is exceeded. Otherwise, they’re locked in the vesting contract. This logic seems correct; ensure thorough tests for boundary conditions.
script/dev/SignTermsMessage.s.sol (1)
9-9: Marking 'run' as 'view' is sensible.
The function only logs a signature and does not mutate state. This change aligns with best practices for read-only logic.
setupLocal.sh (1)
12-18: LGTM! Well-documented help function
The help function clearly describes the available options and their purposes.
script/DeployTokenizer.s.sol (1)
14-14: Verify the initialization sequence
The TimelockedToken implementation is crucial for the StakedLockingCrowdSale. Let's verify that all contracts using this implementation are properly initialized.
Also applies to: 31-33
✅ Verification successful
Based on the shell script results, I can now generate the final response:
TimelockedToken initialization sequence verified and properly implemented
The TimelockedToken implementation is correctly initialized across the codebase:
- All instances follow the same pattern: create TimelockedToken implementation first, then pass it to StakedLockingCrowdSale constructor
- Proper initialization sequence is maintained in both test and deployment scripts
- Integration with TokenVesting is handled correctly where required
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the initialization sequence and usage of TimelockedToken
# Check for TimelockedToken usage in other contracts
echo "Checking TimelockedToken usage:"
ast-grep --pattern 'contract $_ {
$$$
TimelockedToken $_
$$$
}'
# Check for proper initialization patterns
echo "Checking initialization patterns:"
rg -A 5 "TimelockedToken.*new"
Length of output: 4164
subgraph/schema.graphql (1)
123-123: LGTM! Schema change aligns with locking crowdsale implementation
The addition of LOCKING_CROWDSALE to the SaleType enum properly integrates with the existing schema structure, particularly with the TimelockedToken and LockedSchedule entities that handle token locking periods.
subgraph/networks.json (1)
19-22: Verify sepolia deployment
The sepolia address appears to be a real deployment. Let's verify its bytecode matches our contract.
subgraph/makeAbis.sh (1)
12-12: LGTM: ABI extraction for LockingCrowdSale
The addition of LockingCrowdSale ABI extraction aligns with the existing pattern and is correctly implemented.
test/CrowdSalePermissioned.t.sol (2)
69-70: LGTM: TimelockedToken integration
Proper initialization of TimelockedToken and its integration with StakedLockingCrowdSale.
107-107: Verify staking and locking durations
The test uses equal durations (60 days) for both staking and locking periods. Consider adding test cases with different durations to ensure the contract handles varying periods correctly.
script/dev/CrowdSale.s.sol (3)
38-40: LGTM: Deployment configuration
Proper initialization and deployment of TimelockedToken and StakedLockingCrowdSale contracts.
46-47: LGTM: Enhanced logging
Good addition of logging statements for contract addresses, which aids in deployment verification.
155-156: Consider standardizing durations across tests and fixtures
The fixture uses 7-day periods while the test uses 60-day periods. Consider standardizing these durations or documenting the reason for the difference.
subgraph/subgraph.yaml (2)
142-159: LGTM: Event handlers are properly configured
The LockingCrowdSale contract's event handlers are correctly configured to handle all relevant events including Started, Settled, Failed, Bid, LockingContractCreated, and various claim events.
127-127: Verify deployment address configuration
The LockingCrowdSale contract address appears to be using a placeholder value (0xbaadf00dbaadf00dbaadf00dbaadf00dbaadf00d). This needs to be updated with the actual deployed contract address.
test/CrowdSaleLockedTest.t.sol (1)
28-29: LGTM: Proper initialization of TimelockedToken
The TimelockedToken implementation is correctly instantiated and passed to the LockingCrowdSale constructor, following the new contract architecture.
test/CrowdSaleWithNonStandardERC20Test.t.sol (1)
53-54: LGTM: Consistent TimelockedToken implementation
The TimelockedToken setup matches the pattern used in other test files, maintaining consistency across the test suite.
subgraph/abis/IPNFT.json (1)
190-208: Document the purpose and behavior of isPoi function
The newly added isPoi function lacks documentation explaining its purpose and behavior. Since it's marked as pure, it suggests the result is deterministic based solely on the tokenId parameter, but the logic for this determination is unclear.
Let's check for any related documentation or usage:
test/CrowdSaleLockedStakedTest.t.sol (2)
58-60: LGTM: Proper initialization of TimelockedToken
The TimelockedToken implementation is correctly initialized and passed to the StakedLockingCrowdSale constructor.
400-404: LGTM: Proper validation of duration parameters
The test correctly verifies that the staking duration must be less than the vesting duration, ensuring the contract's invariants are maintained.
subgraph/abis/LockingCrowdSale.json (3)
1-12: LGTM: Constructor properly accepts TimelockedToken implementation
The constructor is well-defined with proper input validation through the contract type specification.
463-486: LGTM: Well-structured events with proper indexing
The events are well-designed with:
- Proper indexing of key parameters for efficient filtering
- Comprehensive event data for tracking important state changes
- Clear separation of concerns between different event types
Also applies to: 487-517, 518-543, 544-556, 557-569, 570-588, 589-607, 608-632, 633-780
781-862: LGTM: Comprehensive error handling
The contract includes a complete set of custom errors that:
- Provide specific error cases for better error handling
- Include relevant error parameters where needed
- Use descriptive names that clearly indicate the error condition
subgraph/abis/StakedLockingCrowdSale.json (2)
2-12: LGTM: Constructor properly initializes timelocked token implementation
The constructor correctly accepts and stores the timelocked token implementation address, which is essential for the contract's functionality.
322-340: Verify vesting duration constraints
The addition of salesStakeVestingDuration and stakesVestingDuration parameters requires validation:
- Ensure the vesting duration cannot be set to zero
- Verify the vesting duration cannot exceed reasonable limits
- Consider adding checks for the relationship between locking and vesting durations
Also applies to: 485-489
✅ Verification successful
Vesting duration constraints are properly implemented
The code shows that vesting duration constraints are properly handled:
- Zero duration check:
if (stakesVestingDuration < 7 days) { revert InvalidDuration(); } - Reasonable limits: Minimum of 7 days is enforced as required by TokenVesting contract
- Relationship with locking duration: The implementation correctly handles both durations independently:
- Locking duration for auction tokens is handled by parent contract
- Vesting duration for stakes is enforced to be at least 7 days
- If vesting period has expired, tokens are transferred directly instead of being vested
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for duration validation checks
ast-grep --pattern 'function startSale($$$) {
$$$
require($duration > 0, $_)
$$$
}'
# Look for maximum duration constraints
ast-grep --pattern 'function startSale($$$) {
$$$
require($duration <= $_maximum, $_)
$$$
}'
Length of output: 192
Script:
#!/bin/bash
# Search for Solidity files containing StakedLockingCrowdSale contract
fd -e sol | xargs rg -l "contract StakedLockingCrowdSale"
# Search for duration validation patterns in Solidity files
rg -U "require\(.*duration.*[><=].*\)" --type sol
# Search for vesting duration setter or initialization
ast-grep --pattern 'function set$_VestingDuration($$$) {
$$$
}'
# Look for constructor initialization of vesting duration
ast-grep --pattern 'constructor($$$) {
$$$
$_VestingDuration = $_
$$$
}'
Length of output: 329
Script:
#!/bin/bash
# Let's examine the contract implementation
cat src/crowdsale/StakedLockingCrowdSale.sol
# Also search for any validation patterns with require
rg "require\(" src/crowdsale/StakedLockingCrowdSale.sol
Length of output: 10148
Signed-off-by: Stefan Adolf <stefan@molecule.to>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/crowdsale/LockingCrowdSale.sol (1)
30-34: Consider splitting the Started event for better gas efficiency.The Started event contains many parameters which could increase gas costs when emitting. Consider splitting it into two events:
- One for basic sale information
- One for locking-specific information
-event Started(uint256 indexed saleId, address indexed issuer, Sale sale, TimelockedToken lockingToken, uint256 lockingDuration, uint16 feeBp); +event SaleStarted(uint256 indexed saleId, address indexed issuer, Sale sale, uint16 feeBp); +event LockingConfigured(uint256 indexed saleId, TimelockedToken lockingToken, uint256 lockingDuration);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/crowdsale/LockingCrowdSale.sol(4 hunks)
🔇 Additional comments (2)
src/crowdsale/LockingCrowdSale.sol (2)
Line range hint 4-16: LGTM! Imports are well-organized and appropriate.
The imports are properly scoped and include all necessary dependencies for the contract's functionality.
Line range hint 4-136: Verify reentrancy protection across token operations.
The contract handles token transfers and external calls. Let's verify the implementation of reentrancy guards.
✅ Verification successful
Let me gather more information about the base contract and its inheritance to verify reentrancy protection.
Based on the results, I can now generate the final response as we have sufficient information about the reentrancy protection:
Reentrancy protection is properly implemented.
The contract inherits from OpenZeppelin's ReentrancyGuard through its base CrowdSale contract, and critical functions that handle tokens are protected with the nonReentrant modifier:
settle()function is protectedclaim()function is protected- Token operations in
LockingCrowdSaleare performed through internal functions called from these protected public functions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential reentrancy vulnerabilities in token operations
# Check for nonReentrant modifiers in functions that handle tokens
echo "Checking for reentrancy protection in token handling functions..."
rg -A 2 "safeTransfer|forceApprove|lock" src/crowdsale/LockingCrowdSale.sol
# Check if the base CrowdSale contract implements reentrancy protection
echo "Checking base contract for reentrancy protection..."
ast-grep --pattern 'contract CrowdSale {
$$$
modifier nonReentrant() {
$$$
}
$$$
}'
Length of output: 5353
Script:
#!/bin/bash
# Check CrowdSale.sol for ReentrancyGuard inheritance and usage
echo "Checking CrowdSale.sol imports and inheritance..."
rg -A 5 "contract CrowdSale" src/crowdsale/CrowdSale.sol
# Check for ReentrancyGuard usage in functions that handle tokens in CrowdSale
echo "Checking token handling functions in CrowdSale..."
rg -A 2 "nonReentrant|ReentrancyGuard" src/crowdsale/CrowdSale.sol
# Check implementation of _claimAuctionTokens in base contract
echo "Checking _claimAuctionTokens implementation..."
ast-grep --pattern 'function _claimAuctionTokens($$$) {
$$$
}'
Length of output: 1519
subgraph mapping code reuse configures an slcs2 contract 🤷 uses yaml refs to reuse slsc handlers Signed-off-by: Stefan Adolf <stefan@molecule.to>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
subgraph/subgraph.yaml (1)
214-215: Remove trailing spacesRemove trailing spaces from line 215 to maintain consistent formatting.
- mapping: *stakedLockingCrowdSaleMapping - + mapping: *stakedLockingCrowdSaleMapping🧰 Tools
🪛 yamllint (1.35.1)
[error] 215-215: trailing spaces
(trailing-spaces)
subgraph/src/lockingCrowdSaleMapping.ts (1)
48-60: Ensure locked token uniqueness
This logic sets the locked token if unset and logs an error if the locked token is mismatched. Consider whether to escalate mismatches beyond a log error (e.g., ignoring subsequent updates or halting processing) for data integrity.subgraph/src/stakedLockingCrowdSaleMapping.ts (1)
68-71: Potential fragility in parameter indexing
Accessing parameters by numeric index (like 7 for feeBp) can be error-prone if the event signature changes. Consider using a struct or referencing named fields if possible.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
script/prod/RolloutV25Sale.sol(1 hunks)src/crowdsale/LockingCrowdSale.sol(4 hunks)subgraph/networks.json(3 hunks)subgraph/src/lockingCrowdSaleMapping.ts(1 hunks)subgraph/src/stakedLockingCrowdSaleMapping.ts(4 hunks)subgraph/subgraph.yaml(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- script/prod/RolloutV25Sale.sol
🧰 Additional context used
🪛 Biome (1.9.4)
subgraph/src/lockingCrowdSaleMapping.ts
[error] 2-2: Do not shadow the global "BigInt" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🪛 yamllint (1.35.1)
subgraph/subgraph.yaml
[error] 181-181: trailing spaces
(trailing-spaces)
[error] 215-215: trailing spaces
(trailing-spaces)
🔇 Additional comments (23)
subgraph/networks.json (2)
53-56: Placeholder mainnet address still not updated
The placeholder address for LockingCrowdSale that was flagged in a previous review still needs to be addressed.
Line range hint 19-94: Verify start block numbers across networks
Please verify that the start block numbers are correct for the new contracts:
- Sepolia: 7306191
- Mainnet: 21431344
- Foundry: 0
These should correspond to the actual block numbers where these contracts were deployed.
subgraph/subgraph.yaml (4)
143-159: LGTM: Event handlers configuration
The event handlers are well-structured and include all necessary events for the LockingCrowdSale functionality, including the new LockingContractCreated event.
90-90: Verify CrowdSale contract deployment address
Please ensure this address matches the deployed CrowdSale contract on the intended network.
✅ Verification successful
CrowdSale contract address is correctly configured for local development
The address 0x09635F643e140090A9A8Dcd712eD6285858ceBef is properly referenced in the foundry network configuration in networks.json, which is used for local development. This matches the address in the subgraph configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify contract deployment and bytecode match
# Check if contract exists and has bytecode at the specified address
cast code 0x09635F643e140090A9A8Dcd712eD6285858ceBef --rpc-url http://localhost:8545
Length of output: 181
Script:
#!/bin/bash
# Let's verify if this address is referenced in other configuration files or deployment scripts
# This can help validate if the address is consistently used across the project
# Search for the address in all files
rg -i "0x09635F643e140090A9A8Dcd712eD6285858ceBef"
# Check for any deployment scripts or configuration files
fd -e js -e ts -e json "deploy|config"
Length of output: 374
Script:
#!/bin/bash
# Let's check the networks.json file to understand the network context of this address
cat subgraph/networks.json
# Also check hardhat config for network configurations
cat hardhat.config.js
Length of output: 3360
166-166: Verify StakedLockingCrowdSale contract deployment address
Please ensure this address matches the deployed StakedLockingCrowdSale contract on the intended network.
Line range hint 4-215: Verify network and startBlock configurations
Please confirm that using:
- network: "foundry"
- startBlock: 0
is intentional for all contracts. These settings will affect the subgraph's indexing behavior.
✅ Verification successful
Let me verify the configuration settings by checking the repository for any related configuration files or documentation.
The results will help us understand if these network settings are consistently used across the project and if they are documented somewhere.
Let me check the subgraph-specific documentation and configuration to understand if there's any guidance about the network and startBlock settings.
Let me correct the syntax and try again with proper glob patterns.
Let me check the setup script and environment configuration to understand how the network settings are managed.
Based on the gathered information from the repository, I can now provide a final response:
Network and startBlock configurations are correctly set for local development
The use of "foundry" network and startBlock 0 is intentional as this configuration is specifically for local development:
- The README confirms this is a development setup using Foundry/Anvil as the local testnet
- Separate configurations exist for production networks (Sepolia and Mainnet) as shown in the subgraph's README
- StartBlock 0 is appropriate for local development where you want to index from the chain's beginning
For production deployments, the subgraph is prepared with different configurations using yarn prepare:local for local development and likely other prepare scripts for mainnet/testnet deployments.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Check for network configuration in other yaml files
fd -e yaml -e yml | xargs grep -l "network:\|foundry"
# Look for documentation about network configuration
fd "README.md" | xargs grep -i "network\|foundry"
# Check if there are different network configurations in different branches or environments
fd "subgraph.yaml" | xargs grep "network:"
Length of output: 3234
Script:
#!/bin/bash
# Check subgraph documentation
fd -t f -p "**/subgraph/**/README.md" | xargs cat
# Check for any environment-specific subgraph configurations
fd -t f -p "**/subgraph/**/*.example" | xargs cat
# Look for any deployment scripts that might contain network configuration
fd -t f -p "**/subgraph/**/deploy" -p "**/subgraph/**/*.sh" | xargs cat
Length of output: 892
Script:
#!/bin/bash
# Check subgraph documentation
cat subgraph/README.md 2>/dev/null
# Look for setup or deployment scripts
fd -t f "setup.sh|deploy.sh" subgraph/
# Check for any environment files
fd -t f ".env" subgraph/
# Look for any configuration related files
fd -t f "config|manifest" subgraph/
Length of output: 2798
Script:
#!/bin/bash
# Check setup script content
cat subgraph/setup.sh
# Check for environment example file
cat subgraph/.env.example 2>/dev/null
# Look for any prepare scripts that might handle network configuration
fd -t f "prepare" subgraph/ -x cat {}
Length of output: 1189
subgraph/src/lockingCrowdSaleMapping.ts (5)
2-2: False positive on BigInt overshadowing
This usage of "BigInt" from the Graph Protocol is typical and does not shadow the global BigInt in JavaScript.
🧰 Tools
🪛 Biome (1.9.4)
[error] 2-2: Do not shadow the global "BigInt" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
31-39: Validate crowdSale entity existence
This block properly checks for a missing CrowdSale entity before proceeding. Good error handling to avoid null references.
62-68: Initialization of staked amount and type
Setting amountStaked to zero and assigning the crowdSale type to "LOCKING_CROWDSALE" is clear and consistent.
70-89: Combining plain and locking handleStarted
Invoking both plainHandleStarted and lockingHandleStarted ensures that the event logic for plain and locking sales is consistently applied. The approach looks good for reusability.
123-133: Dynamic data source creation for TimelockedToken
Creating a TimelockedToken template with a context is an established pattern in The Graph. The usage of makeERC20Token and makeTimelockedToken is organized and readable.
subgraph/src/stakedLockingCrowdSaleMapping.ts (6)
24-24: LockingStartedEvent import
Importing the LockingStartedEvent keeps the naming consistent with lockingCrowdSaleMapping. No issues found.
27-27: Reusing functions from lockingCrowdSaleMapping
Centralizing logic in lockingCrowdSaleMapping fosters maintainability, though you might ensure consistent naming to reduce confusion in future updates.
36-38: Helpful docstring
Documenting the legacy event scenario clarifies why fees are omitted. This prevents confusion for future maintainers.
78-96: Creating the LockingStartedEvent
Cloning and dispatching the event for locking logic is consistent with the approach in handleStarted. Just ensure the parameter indexes remain accurate when the underlying event changes.
140-151: Delegating claimed logic
handleClaimed simply reuses the generic logic, maintaining consistent behavior across different crowd sale types. Good separation of concerns.
153-157: Delegated handling of lockingContractCreated
Re-routing to lockedHandleLockingContractCreated centralizes the creation logic and avoids code duplication.
src/crowdsale/LockingCrowdSale.sol (6)
4-4: OpenZeppelin imports
Using IERC20 for address-based validations is standard. No immediate concerns.
Line range hint 16-29: Comprehensive contract documentation
The docstring accurately describes LockingCrowdSale’s purpose. This improves readability and eases onboarding for new contributors.
30-30: Public immutable variable
Exposing TIMELOCKED_TOKEN_IMPLEMENTATION as public is convenient for transparency. Ensure it’s not left uninitialized or zero in the constructor.
35-37: Add zero address check in the constructor
Validating that _timelockedTokenImplementation is nonzero prevents accidental deployments with invalid addresses.
constructor(TimelockedToken _timelockedTokenImplementation) {
+ require(address(_timelockedTokenImplementation) != address(0), "Invalid implementation address");
TIMELOCKED_TOKEN_IMPLEMENTATION = address(_timelockedTokenImplementation);
}44-52: Add critical validations to trustLockingContract
You may want to verify nonzero addresses, ensure no prior registrations, and optionally add an event to capture newly trusted locking contracts.
function trustLockingContract(IERC20 token, TimelockedToken _timelockedToken) public onlyOwner {
+ require(address(token) != address(0), "Invalid token address");
+ require(address(_timelockedToken) != address(0), "Invalid TimelockedToken address");
+ require(address(lockingContracts[address(token)]) == address(0), "Locking contract already trusted");
lockingContracts[address(token)] = _timelockedToken;
+ emit LockingContractTrusted(address(token), address(_timelockedToken));
}
+event LockingContractTrusted(address indexed token, address indexed lockingContract);133-133: Add error handling for the token initialization
Initialization can fail due to unexpected token behavior. Consider using try-catch for improved error diagnosis.
lockedTokenContract = TimelockedToken(Clones.clone(TIMELOCKED_TOKEN_IMPLEMENTATION));
-lockedTokenContract.initialize(auctionToken);
+try lockedTokenContract.initialize(auctionToken) {
+ emit LockingContractCreated(lockedTokenContract, auctionToken);
+} catch Error(string memory reason) {
+ revert(string.concat("Initialization failed: ", reason));
+} catch {
+ revert("Initialization failed");
+}Signed-off-by: Stefan Adolf <stefan@molecule.to>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
README.md (1)
212-214: Fix documentation formattingThere are some minor issues in the verification instructions:
- Remove duplicate word "crowdsale"
- Fix article usage ("a the" → "the")
-Verifying a the staked crowdsale crowdsale +Verifying the staked crowdsale🧰 Tools
🪛 LanguageTool
[grammar] ~212-~212: Two determiners in a row. Choose either “a” or “the”.
Context: ....sol:StakedLockingCrowdSaleVerifying a the staked crowdsale crowdsaleforge verif...(DT_DT)
[duplication] ~212-~212: Possible typo: you repeated a word
Context: ...ckingCrowdSaleVerifying a the staked crowdsale crowdsaleforge verify-contract --chain-id=11155...(ENGLISH_WORD_REPEAT_RULE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(2 hunks)script/prod/RolloutV25Sale.sol(1 hunks)
🧰 Additional context used
🪛 Markdownlint (0.37.0)
README.md
54-54: null
Bare URL used
(MD034, no-bare-urls)
🪛 Gitleaks (8.21.2)
README.md
52-52: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 LanguageTool
README.md
[grammar] ~212-~212: Two determiners in a row. Choose either “a” or “the”.
Context: ....sol:StakedLockingCrowdSale Verifying a the staked crowdsale crowdsaleforge verif...
(DT_DT)
[duplication] ~212-~212: Possible typo: you repeated a word
Context: ...ckingCrowdSale Verifying a the staked crowdsale crowdsaleforge verify-contract --chain-id=11155...
(ENGLISH_WORD_REPEAT_RULE)
🔇 Additional comments (1)
README.md (1)
49-51: LGTM: Contract addresses documented
The new contract addresses for Locking Crowdsale and Staked Crowdsale are properly documented with verification links.
Signed-off-by: Stefan Adolf <stefan@molecule.to>
elmariachi111
force-pushed
the
feature/hubs-224-crowdsale-with-individual-locking-staking-periods
branch
from
3dcd036 to
48afc39
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
subgraph/src/stakedLockingCrowdSaleMapping.ts (1)
66-93: Add type information to parameter comments.The comments for parameters could be more descriptive, especially for complex types like
struct Sale. Consider adding the full structure definition in the comments for better maintainability.- event.parameters[2], // struct Sale + event.parameters[2], // struct Sale { + // address auctionToken; + // uint256 startTime; + // uint256 endTime; + // ... // complete the structure + // }subgraph/abis/LockingCrowdSale.json (4)
1-12: Consider improving parameter naming conventionThe constructor parameter
_timelockedTokenImplementationuses an underscore prefix. While this is common in Solidity, it's generally better to use consistent naming without underscore prefixes for parameters.- "name": "_timelockedTokenImplementation", + "name": "timelockedTokenImplementation",
626-772: Consider consolidating Started eventsThere are two
Startedevents with different parameter sets. This could cause confusion for event listeners and make it harder to track sale starts consistently. Consider:
- Consolidating into a single event with optional parameters
- Using more distinct names to clarify the difference (e.g.,
StartedWithLockingandStartedWithoutLocking)
773-854: Enhance error messages with additional contextConsider adding parameters to the following errors to provide more context:
BadDecimals: Add expected and actual decimalsBadSaleDuration: Add minimum and maximum allowed durationsBadSalesAmount: Add minimum and maximum allowed amountsBidTooLow: Add minimum required bid amountFeesTooHigh: Add maximum allowed fee
281-292: Add maximum fee cap safeguardThe
setCurrentFeesBpfunction allows the owner to set fees without an explicit maximum cap in the ABI. While there's aFeesTooHigherror, consider:
- Adding a constant for maximum allowed fees
- Documenting the fee limits in the ABI
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
subgraph/abis/LockingCrowdSale.json(1 hunks)subgraph/abis/StakedLockingCrowdSale.json(4 hunks)subgraph/networks.json(3 hunks)subgraph/package.json(1 hunks)subgraph/schema.graphql(2 hunks)subgraph/src/crowdSaleMapping.ts(1 hunks)subgraph/src/stakedLockingCrowdSaleMapping.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- subgraph/src/crowdSaleMapping.ts
- subgraph/schema.graphql
- subgraph/networks.json
🔇 Additional comments (9)
subgraph/package.json (1)
11-11: Version bump aligns with new features.
The version change from 1.2.1 to 1.3.0-dev.1 appropriately reflects the addition of new features (TimelockedToken, StakedLockingCrowdSale) while indicating this is a development release for testing on Sepolia.
Let's verify the contract changes that justify this version bump:
✅ Verification successful
The shell script results confirm the presence of new contract implementations (StakedLockingCrowdSale.sol and LockingCrowdSale.sol) and their deployments in the networks configuration. Let me verify the subgraph schema changes to ensure they support these new contracts.
Let me check the subgraph mappings in a different way to verify the handlers for the new contracts.
Based on the shell script results, I can now generate the final response. The code shows:
- New contract implementations for
StakedLockingCrowdSaleandLockingCrowdSale - Corresponding subgraph mappings (
lockingCrowdSaleMapping.tsandstakedLockingCrowdSaleMapping.ts) - Schema updates to support new crowdsale types (
SaleTypeenum withLOCKING_CROWDSALEandSTAKED_LOCKING_CROWDSALE) - Deployed contract addresses in networks configuration
Version bump aligns with new features and implementation.
The version change from 1.2.1 to 1.3.0-dev.1 correctly reflects:
- Minor version bump (1.3.0) for new features (LockingCrowdSale and StakedLockingCrowdSale)
- Dev suffix (-dev.1) indicating development/testing phase on Sepolia
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the contract changes that justify the version bump
# Check for new contract implementations
echo "Checking for new contract implementations..."
rg -l "contract.*?(TimeLocked|StakedLocking|LockingCrowdSale)"
# Check for contract deployment configurations
echo "Checking deployment configurations..."
fd "networks.json|addresses.json" --exec cat {}
Length of output: 3425
Script:
#!/bin/bash
# Check subgraph schema and mapping changes
echo "Checking subgraph schema..."
cat subgraph/schema.graphql
echo -e "\nChecking subgraph mappings for new handlers..."
rg -l "class.*(TimeLocked|StakedLocking|LockingCrowdSale)" subgraph/src/
Length of output: 5131
Script:
#!/bin/bash
# Check for mapping files and their content
echo "Listing mapping files..."
fd "\.ts$" subgraph/src/
echo -e "\nChecking mapping content for new handlers..."
fd "\.ts$" subgraph/src/ --exec cat {}
Length of output: 33724
subgraph/abis/StakedLockingCrowdSale.json (4)
2-25: LGTM: Constructor and implementation getter are well-defined
The constructor properly accepts the timelocked token implementation address, and the corresponding getter function follows the constant/immutable pattern naming convention.
309-327: LGTM: Vesting duration getter follows existing patterns
The salesStakeVestingDuration function maintains consistency with the existing salesLockingDuration pattern for duration management.
618-635: Security review needed for trustLockingContract
The trustLockingContract function introduces trust relationships that require careful security considerations:
- Access control mechanisms
- Trust revocation process
- Event emission for transparency
#!/bin/bash
# Description: Verify security controls around trustLockingContract
# Check for access control modifiers
ast-grep --pattern 'function trustLockingContract($$$) {
$$$
}'
# Look for related events
rg "event.*Trust" --type sol472-476: Verify the impact of the new stakesVestingDuration parameter
The addition of stakesVestingDuration parameter to startSale looks correct. However, we should verify that all existing calls to this function are updated accordingly.
✅ Verification successful
Based on the search results, I can now generate the final response. The code shows that there are three different startSale function signatures in the StakedLockingCrowdSale contract:
- A pure function that reverts with
UnsupportedInitializer - A function inherited from parent that takes
SaleandlockingDuration - The new implementation that takes additional parameters including
stakesVestingDuration
Looking at the test files, all calls to startSale in CrowdSaleLockedStakedTest.t.sol and other test files have been updated to use the new signature with the required parameters. The deployment scripts and production rollout scripts also use the correct signature.
All existing calls to startSale have been properly updated to include the new stakesVestingDuration parameter
- All test files use the correct function signature with required parameters
- Deployment scripts (
RolloutV25Sale.sol,DeployTokenizer.s.sol) use the updated signature - Production scripts are also updated to use the new parameter
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for startSale function calls to ensure they include the new parameter
# Search for startSale function calls
rg "startSale\(" -A 5 --type sol
# Search for test files that might need updates
fd -e sol -e js -e ts | rg -l "startSale"
Length of output: 104
Script:
#!/bin/bash
# Let's try a different approach to find startSale references
# Search for startSale function calls in all files
rg "startSale\(" -A 5
# Also search for test files containing startSale
rg -l "startSale"
# Look for contract deployments or interactions
rg "StakedLockingCrowdSale" -A 5
Length of output: 49775
subgraph/src/stakedLockingCrowdSaleMapping.ts (3)
Line range hint 3-32: LGTM: Import changes are well-structured and necessary.
The new imports support the enhanced functionality for handling both legacy and new contract events.
138-149: LGTM: Clean implementation of claim handling.
The function follows the established pattern and correctly delegates to the generic handler.
151-156: 🛠️ Refactor suggestion
Remove uncertain comment and add type safety checks.
The current implementation has two concerns:
- The comment "xing fingers that this works!" suggests uncertainty and should be replaced with proper documentation.
- The unchecked type casting could be unsafe if the event structures don't match exactly.
Consider adding type safety checks and proper documentation:
export function handleLockingContractCreated(
event: LockingContractCreatedEvent
): void {
- //xing fingers that this works!
+ // Verify event structure before type casting
+ if (!event.parameters || event.parameters.length === 0) {
+ log.error('Invalid event parameters for LockingContractCreated', [])
+ return
+ }
lockedHandleLockingContractCreated(changetype<LockedLockingContractCreatedEvent>(event))
}Let's verify the event structure compatibility:
subgraph/abis/LockingCrowdSale.json (1)
367-422: Verify the pure startSale function implementation
The second startSale overload is marked as pure but takes a Sale struct parameter. This seems incorrect as:
- The function likely needs to read or modify contract state
- Having two overloads with different state mutability could lead to confusion
Signed-off-by: Stefan Adolf <stefan@molecule.to>
Signed-off-by: Stefan Adolf <stefan@molecule.to>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
README.md (1)
106-111: LGTM with minor grammar improvements.The documentation clearly explains the evolution of timelocked tokens and provides detailed verification instructions.
Consider these grammar improvements:
-Verifying a the staked crowdsale crowdsale +Verifying the staked crowdsaleAlso applies to: 218-220
🧰 Tools
🪛 LanguageTool
[uncategorized] ~108-~108: Possible missing comma found.
Context: ...ery elegant (and certainly not correct) solution we decided to "trust" external locking ...(AI_HYDRA_LEO_MISSING_COMMA)
[typographical] ~108-~108: It appears that a comma is missing.
Context: ...another crowdsale instance. During this upgrade we decided to externalize the timelock ...(DURING_THAT_TIME_COMMA)
[uncategorized] ~108-~108: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ... externalize the timelock token template so upcoming instances can be verified on c...(COMMA_COMPOUND_SENTENCE_2)
🪛 Markdownlint (0.37.0)
110-110: Expected: ---; Actual: ---
Horizontal rule style(MD035, hr-style)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.md(3 hunks)script/prod/RolloutV25Sale.sol(1 hunks)subgraph/networks.json(3 hunks)subgraph/src/crowdSaleMapping.ts(1 hunks)subgraph/subgraph.yaml(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- subgraph/src/crowdSaleMapping.ts
- subgraph/networks.json
🧰 Additional context used
🪛 Markdownlint (0.37.0)
README.md
54-54: null
Bare URL used
(MD034, no-bare-urls)
110-110: Expected: ---; Actual: ---
Horizontal rule style
(MD035, hr-style)
🪛 Gitleaks (8.21.2)
README.md
52-52: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 LanguageTool
README.md
[uncategorized] ~108-~108: Possible missing comma found.
Context: ...ery elegant (and certainly not correct) solution we decided to "trust" external locking ...
(AI_HYDRA_LEO_MISSING_COMMA)
[typographical] ~108-~108: It appears that a comma is missing.
Context: ...another crowdsale instance. During this upgrade we decided to externalize the timelock ...
(DURING_THAT_TIME_COMMA)
[uncategorized] ~108-~108: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ... externalize the timelock token template so upcoming instances can be verified on c...
(COMMA_COMPOUND_SENTENCE_2)
[grammar] ~218-~218: Two determiners in a row. Choose either “a” or “the”.
Context: ....sol:StakedLockingCrowdSale Verifying a the staked crowdsale crowdsaleforge verif...
(DT_DT)
[duplication] ~218-~218: Possible typo: you repeated a word
Context: ...ckingCrowdSale Verifying a the staked crowdsale crowdsaleforge verify-contract --chain-id=11155...
(ENGLISH_WORD_REPEAT_RULE)
🪛 yamllint (1.35.1)
subgraph/subgraph.yaml
[error] 181-181: trailing spaces
(trailing-spaces)
[error] 216-216: trailing spaces
(trailing-spaces)
🔇 Additional comments (7)
script/prod/RolloutV25Sale.sol (3)
31-42: Critical: Implement ownership transferThe ownership transfer to the multisig is commented out:
//lockingCrowdsale.transferOwnership(moleculeDevMultisig);This is a security risk as the deployer will retain control.
45-64:⚠️ Potential issueCritical: Complete trust relationships and ownership setup
Multiple critical configurations are commented out or incomplete:
- Missing token address in
trustLockingContract- Ownership transfer is commented out
- Commented code at the bottom suggests incomplete setup
Apply this diff to implement the missing configurations:
contract RolloutV25StakedSale is Script { function run() public { TokenVesting vesting = TokenVesting(0x8f80d1183CD983B01B0C9AC6777cC732Ec9800de); vm.startBroadcast(); StakedLockingCrowdSale stakedLockingCrowdSale = new StakedLockingCrowdSale(timelockedTokenImplementation); vesting.grantRole(vesting.ROLE_CREATE_SCHEDULE(), address(stakedLockingCrowdSale)); - //stakedLockingCrowdSale.trustLockingContract(IERC20()); + stakedLockingCrowdSale.trustLockingContract(IERC20(moleculeDevMultisig)); stakedLockingCrowdSale.trustVestingContract(vesting); - //stakedLockingCrowdSale.transferOwnership(moleculeDevMultisig); + stakedLockingCrowdSale.transferOwnership(moleculeDevMultisig); vm.stopBroadcast(); console.log("STAKED_LOCKING_CROWDSALE_ADDRESS=%s", address(stakedLockingCrowdSale)); console.log("timelocked token implementation=%s", address(timelockedTokenImplementation)); - // vestedDaoToken.grantRole(vestedDaoToken.ROLE_CREATE_SCHEDULE(), address(stakedLockingCrowdSale)); - // stakedLockingCrowdSale.trustVestingContract(vestedDaoToken); } }Likely invalid or redundant comment.
15-18: Verify contract addresses before mainnet deploymentThe hardcoded addresses are currently set for Sepolia testnet. Before deploying to mainnet:
- Update
moleculeDevMultisigto the commented mainnet address (0xCfA0F84660fB33bFd07C369E5491Ab02C449f71B)- Update
timelockedTokenImplementationto the appropriate mainnet deployment✅ Verification successful
Verify contract addresses before mainnet deployment
The review comment is accurate. The code follows the established pattern of using Sepolia addresses during development with clear documentation of mainnet addresses. The mainnet
moleculeDevMultisigaddress (0xCfA0F84660fB33bFd07C369E5491Ab02C449f71B) is consistently used across the codebase in mainnet deployments, as evidenced in RolloutV23.sol and other mainnet configurations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the contract addresses are valid and deployed # Check if addresses are valid contracts for addr in "0xF8F79c1E02387b0Fc9DE0945cD9A2c06F127D851" "0x9d5a6ae551f1117946FF6e0e86ef9A1B20C90Cb0"; do echo "Checking $addr..." # Get contract bytecode to verify it's a contract cast code $addr done # Search for any other occurrences of these addresses echo -e "\nSearching for address usage..." rg -A 3 "0xF8F79c1E02387b0Fc9DE0945cD9A2c06F127D851|0x9d5a6ae551f1117946FF6e0e86ef9A1B20C90Cb0"Length of output: 2070
Script:
#!/bin/bash # Search for deployment configurations and environment-specific addresses echo "Searching for deployment configurations..." fd -e json -e js -e ts -e yaml -e yml -e toml | xargs rg -l "address|0x" echo -e "\nSearching for environment configurations..." fd "config|env" | xargs rg -l "address|0x" echo -e "\nSearching for other occurrences of the mainnet address..." rg "0xCfA0F84660fB33bFd07C369E5491Ab02C449f71B"Length of output: 1680
Script:
#!/bin/bash echo "Checking environment configurations..." cat subgraph/config/mainnet.js cat subgraph/config/sepolia.js echo -e "\nChecking RolloutV23.sol for context..." cat script/prod/RolloutV23.solLength of output: 3597
subgraph/subgraph.yaml (3)
90-90: LGTM!The CrowdSale contract address has been updated correctly.
Line range hint
166-216: LGTM! Well-structured configuration with forward compatibility.The configuration properly handles:
- Both legacy and current Started events
- All required event handlers
- Provides a clear template for future crowdsale additions
🧰 Tools
🪛 yamllint (1.35.1)
[error] 216-216: trailing spaces
(trailing-spaces)
122-160:⚠️ Potential issueUpdate placeholder contract address
The address is currently set to a placeholder value (0xbaadf00dbaadf00dbaadf00dbaadf00dbaadf00d). This needs to be updated with the actual deployed contract address.
README.md (1)
49-54: LGTM! Well-documented contract updates.The changes properly document:
- New Locking Crowdsale deployment
- Updated Staked Crowdsale address
- Timelocked token implementation
- Historical reference to previous deployment
🧰 Tools
🪛 Markdownlint (0.37.0)
54-54: null
Bare URL used(MD034, no-bare-urls)
🪛 Gitleaks (8.21.2)
52-52: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
Signed-off-by: Stefan Adolf <stefan@molecule.to>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
script/prod/RolloutV25Sale.s.sol (3)
15-15: Parameterize hardcoded addresses for flexibility and maintainabilityThe script uses hardcoded addresses for
timelockedTokenImplementation(line 15),moleculeDevMultisig(line 18), andvesting(line 48). Using hardcoded addresses can lead to issues when deploying to different networks or environments. Consider parameterizing these addresses, possibly through configuration files or environment variables, to enhance flexibility and ease of deployment across various environments.Also applies to: 18-18, 48-48
53-53: Remove obsolete commented code for clarityThere are several lines of commented-out code that may no longer be necessary or relevant (lines 53, 60-62). Removing obsolete code improves readability and maintainability of the script.
Apply this diff to remove the unnecessary code:
- //stakedLockingCrowdSale.trustLockingContract(IERC20()); - // 0x7c36c64DA1c3a2065074caa9C48e7648FB733aAB - // vestedDaoToken.grantRole(vestedDaoToken.ROLE_CREATE_SCHEDULE(), address(stakedLockingCrowdSale)); - // stakedLockingCrowdSale.trustVestingContract(vestedDaoToken);Also applies to: 60-62
5-5: Redundant import ofconsole.sol
console.solis imported explicitly on line 5, but sinceScript.solimported on line 4 already includesconsole.sol, the separate import may be redundant. Removing the redundant import helps reduce unnecessary code.Apply this diff to remove the redundant import:
- import "forge-std/console.sol";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
script/prod/RolloutV25Sale.s.sol(1 hunks)subgraph/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- subgraph/package.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Smart contract tests
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
subgraph/networks.json (1)
79-82:⚠️ Potential issueCritical: Replace placeholder foundry address
The foundry address for
LockingCrowdSaleis using a placeholder value (0xbaadf00d...). While this is acceptable for testing, ensure it's updated with the actual deployed address.src/crowdsale/LockingCrowdSale.sol (2)
34-36: 🛠️ Refactor suggestionAdd address validation in constructor.
The constructor should validate that the provided TimelockedToken implementation address is not zero to prevent potential deployment issues.
constructor(TimelockedToken _timelockedTokenImplementation) { + require(address(_timelockedTokenImplementation) != address(0), "Invalid implementation address"); TIMELOCKED_TOKEN_IMPLEMENTATION = address(_timelockedTokenImplementation); }
43-51:⚠️ Potential issueAdd critical validations to trustLockingContract function.
The function lacks several important security checks:
- No validation of input addresses
- No check if a contract is already trusted
- No verification that the locking contract is compatible
- Missing event emission for tracking
+event LockingContractTrusted(address indexed token, address indexed lockingContract); function trustLockingContract(IERC20 token, TimelockedToken _timelockedToken) public onlyOwner { + require(address(token) != address(0), "Invalid token address"); + require(address(_timelockedToken) != address(0), "Invalid locking contract address"); + require(address(lockingContracts[address(token)]) == address(0), "Locking contract already exists"); + + // Verify the locking contract is compatible + require( + _timelockedToken.UNDERLYING_TOKEN() == token, + "Incompatible locking contract" + ); + lockingContracts[address(token)] = _timelockedToken; + emit LockingContractTrusted(address(token), address(_timelockedToken)); }
🧹 Nitpick comments (2)
README.md (2)
110-113: Improve grammar in Timelocked Tokens section.Consider these grammar improvements:
- Add "the" before "beginning of 2025"
- Add a comma after "During this upgrade"
- Add a comma before "so" in the last sentence
-originally the "timelocked token" was an inline concept of the slcs. Timelock contracts weren't reusable among cs impls. This changes as of beginning of 2025. As a rather simple but not very elegant (and certainly not correct) solution we decided to "trust" external locking contracts so you can reuse them among crowdsale instances. +Originally the "timelocked token" was an inline concept of the slcs. Timelock contracts weren't reusable among cs impls. This changes as of the beginning of 2025. As a rather simple but not very elegant (and certainly not correct) solution, we decided to "trust" external locking contracts, so you can reuse them among crowdsale instances.🧰 Tools
🪛 LanguageTool
[uncategorized] ~112-~112: Possible missing article found.
Context: ...able among cs impls. This changes as of beginning of 2025. As a rather simple but not ver...(AI_HYDRA_LEO_MISSING_THE)
[typographical] ~112-~112: It appears that a comma is missing.
Context: ...another crowdsale instance. During this upgrade we decided to externalize the timelock ...(DURING_THAT_TIME_COMMA)
[uncategorized] ~112-~112: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ... externalize the timelock token template so upcoming instances can be verified on c...(COMMA_COMPOUND_SENTENCE_2)
222-224: Fix typo in verification section.Remove the duplicate word "crowdsale" and fix the article usage:
-Verifying a the staked crowdsale crowdsale +Verifying the staked crowdsale🧰 Tools
🪛 LanguageTool
[grammar] ~222-~222: Two determiners in a row. Choose either “a” or “the”.
Context: ....sol:StakedLockingCrowdSaleVerifying a the staked crowdsale crowdsaleforge verif...(DT_DT)
[duplication] ~222-~222: Possible typo: you repeated a word.
Context: ...ckingCrowdSaleVerifying a the staked crowdsale crowdsaleforge verify-contract --chain-id=11155...(ENGLISH_WORD_REPEAT_RULE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.md(4 hunks)script/prod/RolloutV25Sale.s.sol(1 hunks)src/crowdsale/LockingCrowdSale.sol(3 hunks)subgraph/networks.json(3 hunks)subgraph/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- subgraph/package.json
- script/prod/RolloutV25Sale.s.sol
🧰 Additional context used
🪛 Gitleaks (8.21.2)
README.md
20-20: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
56-56: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 Markdownlint (0.37.0)
README.md
58-58: null
Bare URL used
(MD034, no-bare-urls)
114-114: Expected: ---; Actual: ---
Horizontal rule style
(MD035, hr-style)
🪛 LanguageTool
README.md
[uncategorized] ~112-~112: Possible missing article found.
Context: ...able among cs impls. This changes as of beginning of 2025. As a rather simple but not ver...
(AI_HYDRA_LEO_MISSING_THE)
[typographical] ~112-~112: It appears that a comma is missing.
Context: ...another crowdsale instance. During this upgrade we decided to externalize the timelock ...
(DURING_THAT_TIME_COMMA)
[uncategorized] ~112-~112: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ... externalize the timelock token template so upcoming instances can be verified on c...
(COMMA_COMPOUND_SENTENCE_2)
[grammar] ~222-~222: Two determiners in a row. Choose either “a” or “the”.
Context: ....sol:StakedLockingCrowdSale Verifying a the staked crowdsale crowdsaleforge verif...
(DT_DT)
[duplication] ~222-~222: Possible typo: you repeated a word.
Context: ...ckingCrowdSale Verifying a the staked crowdsale crowdsaleforge verify-contract --chain-id=11155...
(ENGLISH_WORD_REPEAT_RULE)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Smart contract tests
🔇 Additional comments (6)
subgraph/networks.json (2)
23-26: LGTM!The Sepolia network configuration for
LockingCrowdSaleis properly defined with a valid address and start block.
53-56: LGTM!The Mainnet network configuration for
LockingCrowdSaleis properly defined with a valid address and start block.src/crowdsale/LockingCrowdSale.sol (2)
4-4: LGTM!The import of
IERC20from OpenZeppelin is appropriate for token operations.
29-29: LGTM!The immutable public variable follows proper naming conventions and visibility.
README.md (2)
17-17: LGTM!The Mainnet Locking Crowdsale entry is properly documented with matching address and thirdweb badge.
53-54: LGTM!The Sepolia contract entries are properly documented with matching addresses and thirdweb badges.
elmariachi111
force-pushed
the
feature/hubs-224-crowdsale-with-individual-locking-staking-periods
branch
from
e9ef600 to
69591bb
Compare
Signed-off-by: Stefan Adolf <stefan@molecule.to>
elmariachi111
force-pushed
the
feature/hubs-224-crowdsale-with-individual-locking-staking-periods
branch
from
69591bb to
46fba8d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/crowdsale/LockingCrowdSale.sol (3)
34-36:⚠️ Potential issueAdd address validation in constructor.
The constructor should validate that the provided TimelockedToken implementation address is not zero to prevent potential deployment issues.
constructor(TimelockedToken _timelockedTokenImplementation) { + require(address(_timelockedTokenImplementation) != address(0), "Invalid implementation address"); TIMELOCKED_TOKEN_IMPLEMENTATION = address(_timelockedTokenImplementation); }
43-51:⚠️ Potential issueAdd critical validations to trustLockingContract function.
The function lacks several important security checks:
- No validation of input addresses
- No check if a contract is already trusted
- No verification that the locking contract is compatible
- Missing event emission for tracking
function trustLockingContract(IERC20 token, TimelockedToken _timelockedToken) public onlyOwner { + require(address(token) != address(0), "Invalid token address"); + require(address(_timelockedToken) != address(0), "Invalid locking contract address"); + require(address(lockingContracts[address(token)]) == address(0), "Locking contract already exists"); + + // Verify the locking contract is compatible + require( + _timelockedToken.UNDERLYING_TOKEN() == token, + "Incompatible locking contract" + ); + lockingContracts[address(token)] = _timelockedToken; + emit LockingContractTrusted(address(token), address(_timelockedToken)); } +event LockingContractTrusted(address indexed token, address indexed lockingContract);
132-135:⚠️ Potential issueAdd error handling for external calls in _makeNewLockedTokenContract.
The initialization call could fail but there's no error handling. Consider adding try-catch to handle potential initialization failures gracefully.
function _makeNewLockedTokenContract(IERC20Metadata auctionToken) private returns (TimelockedToken lockedTokenContract) { lockedTokenContract = TimelockedToken(Clones.clone(TIMELOCKED_TOKEN_IMPLEMENTATION)); - lockedTokenContract.initialize(auctionToken); + try lockedTokenContract.initialize(auctionToken) { + emit LockingContractCreated(lockedTokenContract, auctionToken); + } catch Error(string memory reason) { + revert(string.concat("Initialization failed: ", reason)); + } catch { + revert("Initialization failed"); + } - emit LockingContractCreated(lockedTokenContract, auctionToken); }
🧹 Nitpick comments (1)
README.md (1)
112-112: Fix grammatical issues in documentation.Several grammatical issues need to be addressed:
- Missing article: "as of the beginning of 2025"
- Missing comma: "solution, we decided"
- Missing comma: "upgrade, we decided"
- Missing comma before 'so'
Apply these corrections:
-originally the "timelocked token" was an inline concept of the slcs. Timelock contracts weren't reusable among cs impls. This changes as of beginning of 2025. As a rather simple but not very elegant (and certainly not correct) solution we decided to "trust" external locking contracts so upcoming instances can be verified on chain. +Originally, the "timelocked token" was an inline concept of the SLCs. Timelock contracts weren't reusable among crowdsale implementations. This changes as of the beginning of 2025. As a rather simple but not very elegant (and certainly not correct) solution, we decided to "trust" external locking contracts, so upcoming instances can be verified on chain.🧰 Tools
🪛 LanguageTool
[uncategorized] ~112-~112: You might be missing the article “the” here.
Context: ...able among cs impls. This changes as of beginning of 2025. As a rather simple but not ver...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~112-~112: A comma might be missing here.
Context: ...ery elegant (and certainly not correct) solution we decided to "trust" external locking ...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[typographical] ~112-~112: It appears that a comma is missing.
Context: ...another crowdsale instance. During this upgrade we decided to externalize the timelock ...(DURING_THAT_TIME_COMMA)
[uncategorized] ~112-~112: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ... externalize the timelock token template so upcoming instances can be verified on c...(COMMA_COMPOUND_SENTENCE_2)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.md(4 hunks)script/prod/RolloutV25Sale.s.sol(1 hunks)src/crowdsale/LockingCrowdSale.sol(3 hunks)subgraph/networks.json(3 hunks)subgraph/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- subgraph/package.json
- script/prod/RolloutV25Sale.s.sol
- subgraph/networks.json
🧰 Additional context used
🪛 Gitleaks (8.21.2)
README.md
20-20: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
56-56: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 Markdownlint (0.37.0)
README.md
58-58: null
Bare URL used
(MD034, no-bare-urls)
114-114: Expected: ---; Actual: ---
Horizontal rule style
(MD035, hr-style)
🪛 LanguageTool
README.md
[uncategorized] ~112-~112: You might be missing the article “the” here.
Context: ...able among cs impls. This changes as of beginning of 2025. As a rather simple but not ver...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~112-~112: A comma might be missing here.
Context: ...ery elegant (and certainly not correct) solution we decided to "trust" external locking ...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[typographical] ~112-~112: It appears that a comma is missing.
Context: ...another crowdsale instance. During this upgrade we decided to externalize the timelock ...
(DURING_THAT_TIME_COMMA)
[uncategorized] ~112-~112: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ... externalize the timelock token template so upcoming instances can be verified on c...
(COMMA_COMPOUND_SENTENCE_2)
🔇 Additional comments (4)
src/crowdsale/LockingCrowdSale.sol (1)
4-4: LGTM! Import changes look good.The addition of IERC20 import from OpenZeppelin is appropriate for the new functionality.
README.md (3)
17-20: Review duplicate crowdsale implementations.There appear to be multiple crowdsale implementations deployed:
- Locking Crowdsale
- StakedLockingCrowdSale
This matches the past review comment about duplicate crowdsales. Consider documenting the differences between these implementations and their specific use cases to avoid confusion.
Let's verify the differences between these implementations:
#!/bin/bash # Description: Compare the implementations of different crowdsale contracts # Expected: Different contract interfaces and functionality # Search for contract definitions ast-grep --pattern 'contract LockingCrowdSale { $$$ }' ast-grep --pattern 'contract StakedLockingCrowdSale { $$$ }'🧰 Tools
🪛 Gitleaks (8.21.2)
20-20: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
110-113: Document technical debt and migration plan.The Timelocked Tokens section acknowledges technical debt:
- "not very elegant (and certainly not correct) solution"
- External locking contracts are being "trusted"
Consider:
- Documenting the known limitations and risks
- Creating a roadmap for addressing these issues
- Adding validation checks for trusted contracts
Would you like me to help create a detailed technical specification for a more robust implementation?
🧰 Tools
🪛 LanguageTool
[uncategorized] ~112-~112: You might be missing the article “the” here.
Context: ...able among cs impls. This changes as of beginning of 2025. As a rather simple but not ver...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~112-~112: A comma might be missing here.
Context: ...ery elegant (and certainly not correct) solution we decided to "trust" external locking ...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[typographical] ~112-~112: It appears that a comma is missing.
Context: ...another crowdsale instance. During this upgrade we decided to externalize the timelock ...(DURING_THAT_TIME_COMMA)
[uncategorized] ~112-~112: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ... externalize the timelock token template so upcoming instances can be verified on c...(COMMA_COMPOUND_SENTENCE_2)
222-224: LGTM: Clear verification instructions added.The new instructions for verifying the staked crowdsale contract are clear and include the necessary constructor arguments.
| | StakedLockingCrowdSale | [0x35Bce29F52f51f547998717CD598068Afa2B29B7](https://etherscan.io/address/0x35Bce29F52f51f547998717CD598068Afa2B29B7#code) | <a href="https://thirdweb.com/ethereum/0x35Bce29F52f51f547998717CD598068Afa2B29B7?utm_source=contract_badge" target="_blank"><img width="200" height="45" src="https://badges.thirdweb.com/contract?address=0x35Bce29F52f51f547998717CD598068Afa2B29B7&theme=dark&chainId=1" alt="View contract" /></a> | | ||
|
|
||
| timelocked token implementation=0x625ed621d814645AA81C50c4f333D4a407576e8F |
There was a problem hiding this comment.
Security: Remove exposed API keys.
The static analysis tool detected potential API keys in the documentation. While these appear to be example/placeholder values, it's best practice to use environment variables or configuration files for such sensitive information.
Replace the API keys with placeholders:
-timelocked token implementation=0x625ed621d814645AA81C50c4f333D4a407576e8F
+timelocked token implementation=${TIMELOCK_IMPLEMENTATION_ADDRESS}Also applies to: 56-56
🧰 Tools
🪛 Gitleaks (8.21.2)
20-20: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
Stefan
Summary by CodeRabbit
Release Notes
New Features
LockingCrowdSalecontract with improved functionality.Improvements
LockingCrowdSaleentries.Bug Fixes
Infrastructure
Documentation