disambiguator on log table#153
Conversation
WalkthroughAdds a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/lib/deploy/LibDecimalFloatDeploy.sol(1 hunks)src/lib/table/LibLogTable.sol(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-30T20:11:57.768Z
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#144
File: src/lib/implementation/LibDecimalFloatImplementation.sol:708-732
Timestamp: 2025-09-30T20:11:57.768Z
Learning: In src/lib/implementation/LibDecimalFloatImplementation.sol, the `lookupLogTableVal` internal function intentionally omits bounds checking for gas optimization because all calling functions perform the necessary validation before invocation (e.g., checking against LOG_MANTISSA_LAST_INDEX in log10).
Applied to files:
src/lib/deploy/LibDecimalFloatDeploy.sol
⏰ 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). (10)
- GitHub Check: deploy
- GitHub Check: rainix (ubuntu-latest, rainix-sol-static)
- GitHub Check: rainix (ubuntu-latest, test-wasm-build)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-static)
- GitHub Check: rainix (macos-latest, rainix-sol-legal)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-test)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-test)
- GitHub Check: rainix (macos-latest, rainix-rs-test)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-legal)
- GitHub Check: git-clean
🔇 Additional comments (1)
src/lib/deploy/LibDecimalFloatDeploy.sol (1)
16-16: LGTM!The import statement correctly brings in the LOG_TABLE_DISAMBIGUATOR constant from the LibLogTable module.
| return abi.encodePacked( | ||
| LOG_TABLES, | ||
| LOG_TABLES_SMALL, | ||
| LOG_TABLES_SMALL_ALT, | ||
| ANTI_LOG_TABLES, | ||
| ANTI_LOG_TABLES_SMALL, | ||
| LOG_TABLE_DISAMBIGUATOR | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
Clarify the collision scenario and consider alternative approaches.
The appended disambiguator successfully alters the deployment address, but the implementation raises some design questions:
-
Versioning pattern: The constant suffix
_1suggests potential future values (_2,_3, etc.), which could become a maintenance burden. Each new value requires updating both the constant definition and deployment logic. -
Cost implications: The extra 32 bytes are stored in the data contract, increasing deployment gas costs. While minimal, this cost is paid on every deployment.
-
Use case clarity: The comment states it avoids collisions "when we actually want distinct addresses," but it's unclear from the code what scenarios require multiple distinct deployments of the same table data.
Consider documenting the specific use case (e.g., "required when deploying to multiple chains with different table versions" or "enables parallel deployments for testing") to help future maintainers understand when and why this is needed.
Run the following script to check for any additional context about deterministic deployments and how the combined tables are consumed:
🏁 Script executed:
#!/bin/bash
# Description: Search for usages of combinedTables() and dataContract() to understand the deployment flow
# Search for calls to combinedTables() or dataContract()
rg -n --type=sol -A 5 -B 2 'combinedTables\(\)|dataContract\(\)' --glob '!src/lib/deploy/LibDecimalFloatDeploy.sol'
# Search for CREATE2 or deterministic deployment patterns
rg -n --type=sol -A 3 'CREATE2|create2|deterministic'
# Search for comments explaining the deployment strategy
rg -n --type=sol -B 2 -A 2 'collision|disambiguator|distinct address'Length of output: 83
🏁 Script executed:
#!/bin/bash
# Search for calls to combinedTables() or dataContract() outside the deploy library
rg -n -g '*.sol' -C3 'combinedTables\(\)|dataContract\(\)' --glob '!src/lib/deploy/LibDecimalFloatDeploy.sol'
# Search for CREATE2 or create2 usage
rg -n -g '*.sol' -C3 'CREATE2|create2'
# Search for comments explaining collision or disambiguator usage
rg -n -g '*.sol' -C3 'collision|disambiguator|distinct address'Length of output: 2725
Reconsider the disambiguator design in combinedTables
- The
_1suffix inLOG_TABLE_DISAMBIGUATORimplies future versions, adding maintenance overhead. - Appending 32 bytes to the payload increases deployment gas; consider leveraging CREATE2’s salt instead of expanding the data.
- Collision rationale is already documented in
src/lib/table/LibLogTable.sol.
🤖 Prompt for AI Agents
In src/lib/deploy/LibDecimalFloatDeploy.sol around lines 20 to 27, the
combinedTables payload currently appends LOG_TABLE_DISAMBIGUATOR (with a _1
suffix) which both bloats deployment calldata and embeds a version-like suffix;
remove the disambiguator from the abi.encodePacked return and stop using the _1
suffix constant here, then switch the uniqueness/collision mechanism to
derive/encode the distinguishing value into the CREATE2 salt at deployment time
instead of into the table payload; update any deployment call sites to pass the
same distinguishing bytes via the CREATE2 salt and remove or rename the
LOG_TABLE_DISAMBIGUATOR constant and its usages so documentation in
src/lib/table/LibLogTable.sol remains the canonical rationale for collision
handling.
| /// @dev As we deterministically deploy the log tables, we can run into | ||
| /// collisions when we actually want distinct addresses. | ||
| bytes32 constant LOG_TABLE_DISAMBIGUATOR = keccak256("LOG_TABLE_DISAMBIGUATOR_1"); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM with documentation suggestion.
The constant is correctly defined with a deterministic keccak256 hash. The use of _1 in the string suggests a versioning scheme, which aligns with the need to generate different addresses for different deployments.
Consider expanding the documentation to briefly mention the specific scenario(s) that require distinct addresses (e.g., "used when deploying different versions of log tables to the same chain" or "enables multiple test deployments without address conflicts").
🤖 Prompt for AI Agents
In src/lib/table/LibLogTable.sol around lines 22 to 24, expand the comment for
LOG_TABLE_DISAMBIGUATOR to state the specific scenarios needing distinct
addresses (e.g., deploying different versions of log tables to the same chain or
running multiple test deployments) so future readers understand why the
disambiguator/version suffix exists; update the docstring to include one short
sentence listing those scenarios and keep the existing constant unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/lib/LibDecimalFloat.sol(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-30T20:11:57.768Z
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#144
File: src/lib/implementation/LibDecimalFloatImplementation.sol:708-732
Timestamp: 2025-09-30T20:11:57.768Z
Learning: In src/lib/implementation/LibDecimalFloatImplementation.sol, the `lookupLogTableVal` internal function intentionally omits bounds checking for gas optimization because all calling functions perform the necessary validation before invocation (e.g., checking against LOG_MANTISSA_LAST_INDEX in log10).
Applied to files:
src/lib/LibDecimalFloat.sol
⏰ 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). (8)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-test)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-static)
- GitHub Check: rainix (ubuntu-latest, test-wasm-build)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-legal)
- GitHub Check: rainix (macos-latest, rainix-sol-legal)
- GitHub Check: rainix (macos-latest, rainix-rs-test)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-test)
- GitHub Check: git-clean
| using LibDecimalFloat for Float; | ||
|
|
||
| address constant LOG_TABLES_ADDRESS = 0xdc564999251875a2728Eec15962c5456C9259f54; | ||
| address constant LOG_TABLES_ADDRESS = 0x6421E8a23cdEe2E6E579b2cDebc8C2A514843593; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the deployment at the new address and ensure table data integrity.
The LOG_TABLES_ADDRESS change is critical as this constant is used by pow10, log10, pow, and sqrt functions. Using an incorrect address or one containing invalid table data would break all logarithmic operations.
Please run the following verification steps:
-
Verify the contract is deployed at the new address and check if it contains the expected disambiguator:
-
Search for deployment scripts or tests that verify the table data:
Additionally, consider documenting the deployment process or adding a verification test that ensures the deployed contract at LOG_TABLES_ADDRESS contains the expected table data structure.
🏁 Script executed:
#!/bin/bash
# Verify deployment and check for LOG_TABLE_DISAMBIGUATOR presence
NEW_ADDR="0x6421E8a23cdEe2E6E579b2cDebc8C2A514843593"
# Check if contract is deployed
CODE=$(cast code "$NEW_ADDR" --rpc-url mainnet 2>/dev/null || echo "")
if [ -z "$CODE" ] || [ "$CODE" = "0x" ]; then
echo "ERROR: No contract deployed at $NEW_ADDR"
exit 1
else
echo "✓ Contract deployed at $NEW_ADDR"
echo "Code size: ${#CODE} bytes"
fi
# Try to read LOG_TABLE_DISAMBIGUATOR if it's exposed
# This assumes the contract has a public getter for the disambiguator
cast call "$NEW_ADDR" "LOG_TABLE_DISAMBIGUATOR()(bytes32)" --rpc-url mainnet 2>/dev/null || echo "Note: Could not read LOG_TABLE_DISAMBIGUATOR (may not be publicly exposed)"Length of output: 73
🏁 Script executed:
#!/bin/bash
# Find deployment and verification scripts
echo "=== Deployment scripts ==="
fd -e sol -e js -e ts . | xargs rg -l "LOG_TABLES_ADDRESS|0x6421E8a23cdEe2E6E579b2cDebc8C2A514843593"
echo -e "\n=== Tests for table deployment ==="
fd -e sol -e js -e ts . | xargs rg -l "LibDecimalFloatDeploy|combinedTables"
echo -e "\n=== LOG_TABLE_DISAMBIGUATOR usage ==="
ast-grep --pattern 'LOG_TABLE_DISAMBIGUATOR'Length of output: 633
LOG_TABLES_ADDRESS not deployed on mainnet
cast code shows no contract at 0x6421E8a23cdEe2E6E579b2cDebc8C2A514843593 on mainnet. This constant underpins pow10, log10, pow, and sqrt—it must point to a live log-table contract. Deploy the tables at this address, update the constant to the correct address, or clarify the target network. Add a test that fails if the address has no deployed code.
|
@coderabbitai assess this PR size classification for the totality of the PR with the following criterias and report it in your comment: S/M/L PR Classification Guidelines:This guide helps classify merged pull requests by effort and complexity rather than just line count. The goal is to assess the difficulty and scope of changes after they have been completed. Small (S)Characteristics:
Review Effort: Would have taken 5-10 minutes Examples:
Medium (M)Characteristics:
Review Effort: Would have taken 15-30 minutes Examples:
Large (L)Characteristics:
Review Effort: Would have taken 45+ minutes Examples:
Additional Factors to ConsiderWhen deciding between sizes, also consider:
Notes:
|
|
Tip For best results, initiate chat on the files or code changes. SIZE=M |
1 participant
Motivation
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit