Add contract for winning probability and hopli command to interact with the new contract #6501
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes encompass the introduction of a new Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Files selected for processing (8)
- ethereum/bindings/Cargo.toml (1 hunks)
- ethereum/contracts/contracts-addresses.json (5 hunks)
- ethereum/contracts/script/DeployAll.s.sol (3 hunks)
- ethereum/contracts/script/utils/NetworkConfig.s.sol (3 hunks)
- ethereum/contracts/src/WinningProbabilityOracle.sol (1 hunks)
- ethereum/contracts/test/WinningProbabilityOracle.t.sol (1 hunks)
- hopr/hopr-lib/data/protocol-config.json (2 hunks)
- scripts/protocol-config-anvil.json (2 hunks)
Additional context used
Gitleaks
ethereum/contracts/contracts-addresses.json
13-13: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
29-29: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
Additional comments not posted (30)
ethereum/bindings/Cargo.toml (1)
3-3: Verify that the version increment aligns with the changes in this PR.Incrementing the patch version from "0.1.0" to "0.1.1" is appropriate if this PR only contains backward-compatible bug fixes or minor changes that do not affect the public API.
Please confirm that the changes in this PR do not introduce any breaking changes or new features, and only include bug fixes or minor changes. If there are any breaking changes or new features, consider incrementing the minor version instead (e.g., "0.2.0").
scripts/protocol-config-anvil.json (2)
13-13: LGTM!The addition of the
"winning_probability_oracle"entry in the configuration file is consistent with the existing structure and aligns with the PR objective of introducing a new contract for winning probability.
35-35: Looks good!The addition of the
"winning_probability_oracle"entry in the"anvil-localhost2"network configuration is consistent with the change made in the"anvil-localhost"configuration.Observation: The duplication of the entry in both network configurations suggests that the
"winning_probability_oracle"contract is intended to be used across multiple local development environments.ethereum/contracts/src/WinningProbabilityOracle.sol (5)
6-11: LGTM!The
WinProbtype andequalfunction are correctly defined and implemented.
13-16: LGTM!The
HoprWinningProbablityOracleEventscontract correctly defines theWinProbUpdatedevent.
44-45: LGTM!The
HoprWinningProbablityOraclecontract correctly inherits fromOwnable2StepandHoprWinningProbablityOracleEvents, and defines thecurrentWinProbstate variable.
54-57: LGTM!The constructor correctly transfers ownership and sets the initial winning probability.
63-79: LGTM!The
setWinProband_setWinProbfunctions are correctly implemented with appropriate access control, input validation, and event emission.ethereum/contracts/test/WinningProbabilityOracle.t.sol (9)
8-11: LGTM!The
Ownable2StepEventscontract correctly defines the necessary events for the two-step ownership transfer process.
17-20: LGTM!The
setUpfunction correctly initializes theoracleinstance with the specified owner and winning probability.
22-25: LGTM!The
test_setUpWithZerofunction verifies that the contract can be initialized with a zero winning probability.
27-32: LGTM!The
test_setZerofunction correctly tests setting the winning probability to zero and verifies the emitted event.
34-38: LGTM!The
testRevert_setSameFailsfunction correctly verifies that setting the same winning probability reverts with the expected error.
40-44: LGTM!The
testRevert_setByNonOwnerFailsfunction correctly verifies that setting the winning probability by a non-owner account reverts with the expected error.
60-69: LGTM!The
test_setALargeValueFailsfunction correctly tests the failure case when trying to set a large value using a low-level call and verifies that the call is unsuccessful.
71-86: LGTM!The
test_transferOwnershipfunction correctly tests the two-step ownership transfer process by initiating the transfer, accepting the ownership, and verifying the emitted events and the new owner.
6-6: Unable to review theHoprWinningProbablityOraclecontract.The
HoprWinningProbablityOraclecontract is imported on line 6 but not provided in the code for review. Please include the contract code to enable a comprehensive review.hopr/hopr-lib/data/protocol-config.json (2)
26-26: LGTM!The new "winning_probability_oracle" entry is added correctly under the "debug-staging" network configuration with a valid Ethereum address.
48-48: LGTM!The new "winning_probability_oracle" entry is added correctly under the "dufour" network configuration with a valid Ethereum address.
ethereum/contracts/contracts-addresses.json (5)
5-5: LGTM!The updated
announcementsaddress for theanvil-localhostenvironment looks good.
14-14: LGTM!The addition of the
winning_probability_oracleaddress for theanvil-localhostenvironment looks good and aligns with the PR objective.
17-17: LGTM!The updated
indexer_start_block_numberfor theanvil-localhostenvironment looks good.
30-30: LGTM!The addition of the
winning_probability_oracleaddress for theanvil-localhost2environment looks good and aligns with the PR objective.
47-47: LGTM!The addition of the
winning_probability_oracleaddresses for therotsee,debug-staging,monte_rosa, anddufourenvironments looks good and aligns with the PR objective. The changes are consistent across all the environments.Also applies to: 63-63, 79-79, 95-95
ethereum/contracts/script/utils/NetworkConfig.s.sol (3)
29-29: LGTM!The addition of the new address variable
winningProbabilityContractAddressto theaddressesstruct is consistent with the existing code structure and naming conventions.
111-111: Looks good!The serialization process is correctly updated to include the new
winningProbabilityContractAddressvariable. The key name and serialization order are consistent with the existing code.
223-232: Approved!The code for writing the
winningProbabilityContractAddressto a file is implemented correctly. It follows the existing patterns and maintains consistency with the surrounding code.ethereum/contracts/script/DeployAll.s.sol (3)
11-11: LGTM!The import statement for the
WinProbtype is correct and necessary for the_deployHoprWinningProbabilityOraclemethod.
91-92: LGTM!The
_deployHoprWinningProbabilityOraclemethod is called with the correct parameters. The defaultWinProbvalue of1.0(maximumuint56value) is a reasonable choice for the initial deployment.
286-298: LGTM!The
_deployHoprWinningProbabilityOraclemethod is implemented correctly. It follows the same deployment logic as other methods in the contract and uses the correct contract name and parameters.
There was a problem hiding this comment.
Actionable comments posted: 15
Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Files selected for processing (8)
- chain/api/src/config.rs (4 hunks)
- chain/types/src/lib.rs (8 hunks)
- ethereum/contracts/contracts-addresses.json (5 hunks)
- ethereum/contracts/test/WinningProbabilityOracle.t.sol (1 hunks)
- hopr/hopr-lib/src/lib.rs (1 hunks)
- hoprd/hoprd/example_cfg.yaml (1 hunks)
- hoprd/hoprd/src/config.rs (1 hunks)
- scripts/protocol-config-anvil.json (2 hunks)
Additional context used
Gitleaks
ethereum/contracts/contracts-addresses.json
13-13: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
29-29: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
Additional comments not posted (18)
scripts/protocol-config-anvil.json (2)
13-13: LGTM!The addition of the
"winning_probability_oracle"entry in the"anvil-localhost"network configuration looks good. The Ethereum address value is likely the address of the deployed "WinningProbability" contract, which aligns with the PR objectives.
35-35: Looks good!The addition of the
"winning_probability_oracle"entry in the"anvil-localhost2"network configuration is consistent with the change made in the"anvil-localhost"network configuration. Using the same Ethereum address value across both configurations ensures uniformity in the system's interaction with the "WinningProbability" contract.ethereum/contracts/contracts-addresses.json (6)
5-17: LGTM!The changes in the
anvil-localhostenvironment look good:
- Updating
indexer_start_block_numberto1is valid for a local environment.- The address updates for existing contracts are expected for a fresh local setup.
- Adding the
winning_probability_oracleaddress aligns with the PR objective.Tools
Gitleaks
13-13: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
21-33: LGTM!The changes in the
anvil-localhost2environment are identical toanvil-localhostand look good for another local environment setup.Tools
Gitleaks
29-29: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
47-47: LGTM!Adding the
winning_probability_oracleaddress in therotseestaging environment looks good and aligns with the PR objective.
63-63: LGTM!Adding the
winning_probability_oracleaddress in thedebug-stagingstaging environment looks good and aligns with the PR objective.
95-95: LGTM!Adding the
winning_probability_oracleaddress in thedufourproduction environment looks good and aligns with the PR objective.
13-13: Dismiss static analysis hints.The lines flagged by Gitleaks as potential generic API keys are false positives. The flagged values are Ethereum addresses for the
winning_probability_oraclein local test environments. Ethereum addresses are not considered sensitive information.Also applies to: 29-29
Tools
Gitleaks
13-13: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
hoprd/hoprd/example_cfg.yaml (1)
249-249: LGTM!The addition of the
winning_probability_oracleentry in the configuration aligns with the PR objective of introducing a new contract for winning probability. The provided Ethereum address indicates the location of the deployed oracle contract.chain/api/src/config.rs (4)
101-103: LGTM!The addition of the
winning_probability_oraclefield to theAddressesstruct is appropriate. The field is correctly typed asAddress, is public for external access, and uses theDisplayFromStrformat for serialization/deserialization.
142-144: Looks good!The addition of the
winning_probability_oraclefield to theChainNetworkConfigstruct is consistent with the changes made to theAddressesstruct. The field is correctly typed, publicly accessible, and uses the appropriate serialization format.
211-211: Looks good!The initialization of the
winning_probability_oraclefield in theChainNetworkConfig::newmethod is consistent with the struct definition. It correctly assigns the value fromnetwork.addresses.winning_probability_oracleand creates an owned copy usingto_owned().
236-236: Looks good!The initialization of the
win_prob_oraclefield in theFrom<&ChainNetworkConfig>implementation forContractAddressesis consistent with the changes made to theChainNetworkConfigstruct. It correctly assigns the value fromnetwork.winning_probability_oracle.hoprd/hoprd/src/config.rs (1)
378-378: LGTM!The addition of the
"winning_probability_oracle"entry in the test configuration looks good. It follows the existing pattern of defining oracle and registry addresses, and it should enhance the testing capabilities related to winning probability scenarios.hopr/hopr-lib/src/lib.rs (1)
552-552: LGTM!The addition of the
win_prob_oraclefield to theContractAddressesstruct is a valid enhancement. It allows theHoprstruct to access the winning probability oracle contract address, which may enable new functionality related to retrieving or calculating winning probabilities for tickets.ethereum/contracts/test/WinningProbabilityOracle.t.sol (2)
40-44: Test correctly ensures only the owner can set win probability.The test verifies that a non-owner cannot call
setWinProband expects the correct revert message.
70-85: Well-structured ownership transfer test.The test correctly verifies both the initiation and acceptance of ownership transfer, including event emission and final owner verification.
chain/types/src/lib.rs (1)
239-239: Ensure consistent field naming in the conversion.Verify that the field
win_prob_oracleis consistently named and correctly spelled throughout the conversion.Run the following script to search for the field usage and check for consistency:
Verification successful
Field naming 'win_prob_oracle' is consistent throughout the codebase.
The verification process confirms that the field 'win_prob_oracle' is consistently named and correctly spelled across multiple files in the Rust codebase. This includes usage in struct definitions, function parameters, and variable assignments. No variations or inconsistencies were found in the naming convention.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent usage of 'win_prob_oracle' in the codebase. # Test: Search for occurrences of 'win_prob_oracle'. Expect: Consistent naming and correct spelling. rg --type rust 'win_prob_oracle'Length of output: 1079
There was a problem hiding this comment.
Actionable comments posted: 5
Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Files selected for processing (4)
- chain/types/src/lib.rs (8 hunks)
- ethereum/contracts/script/DeployAll.s.sol (3 hunks)
- ethereum/contracts/src/WinningProbabilityOracle.sol (1 hunks)
- ethereum/contracts/test/WinningProbabilityOracle.t.sol (1 hunks)
Additional comments not posted (17)
ethereum/contracts/src/WinningProbabilityOracle.sol (9)
1-2: LGTM: Appropriate license and Solidity version.The GPL-3.0 license is suitable for an open-source project, and Solidity version 0.8.19 is a recent and stable version.
13-16: LGTM: Well-structured event definition.The
HoprWinningProbabilityOracleEventsabstract contract is a good practice for organizing events. TheWinProbUpdatedevent properly emits both old and new winning probabilities, which is useful for off-chain monitoring and analysis.
18-44: LGTM: Well-structured contract with clear documentation.The
HoprWinningProbabilityOraclecontract is well-defined, with appropriate inheritance fromOwnable2StepandHoprWinningProbabilityOracleEvents. The detailed comment block provides excellent documentation of the contract's purpose and functionality, which greatly enhances code readability and maintainability.
45-48: LGTM: Appropriate state variable and custom error definition.The
currentWinProbstate variable is correctly declared as public, providing easy access to the current winning probability. The custom errorWinProbMustNotBeSameis a gas-efficient way to handle the case when the new probability is the same as the current one.
50-57: LGTM: Well-implemented constructor.The constructor correctly initializes the contract state by transferring ownership and setting the initial winning probability. The use of the internal
_setWinProbfunction for setting the initial probability is a good practice for code reuse and consistency.
59-68: LGTM: Well-implemented public function with proper access control.The
setWinProbfunction is correctly implemented with the following good practices:
- Restricted to the owner using the
onlyOwnermodifier.- Checks if the new probability is different from the current one, preventing unnecessary updates and gas costs.
- Uses the internal
_setWinProbfunction for the actual update, promoting code reuse.
70-79: LGTM: Well-implemented internal function.The
_setWinProbinternal function is correctly implemented:
- It updates the
currentWinProbstate variable.- It emits the
WinProbUpdatedevent with both old and new probabilities.- Storing the old probability before updating ensures accurate event emission.
This implementation promotes code reuse and maintains consistency in updating the winning probability.
1-80: Overall assessment: Well-implemented contract meeting PR objectives.The
HoprWinningProbabilityOraclecontract successfully implements the winning probability oracle as outlined in the PR objectives and linked issue #6493. Key points:
- Proper access control using OpenZeppelin's
Ownable2Step.- Clear and efficient custom type definition for
WinProb.- Well-structured events for tracking probability updates.
- Comprehensive documentation and inline comments.
- Efficient state management and update mechanism.
- Appropriate error handling and gas optimization.
The contract is well-structured, follows Solidity best practices, and doesn't have any apparent security issues. It's ready for deployment after addressing the minor suggestions in the previous comments.
To ensure full compliance with the PR objectives, please verify that the deployment script and unit tests have been implemented as mentioned in the PR description. You can use the following command to check for the presence of these files:
#!/bin/bash # Check for deployment script and test files fd -e js -e ts "deploy|test" --exec echo "Found file: {}"
6-11: LGTM: Custom type definition enhances code clarity.The custom type
WinProband its equality function are well-implemented, improving code readability and type safety.Could you please clarify the rationale behind using
uint56forWinProb? Is this choice related to gas optimization or compatibility with other parts of the system? Adding a comment explaining this decision would improve code maintainability.Verification successful
Verified:
uint56is uniquely used forWinProb, likely for optimization purposes.The usage of
uint56in theWinProbtype is specific to this context and not found elsewhere in Solidity files within the codebase. This suggests that the choice was intentional, possibly for gas optimization or efficient storage. The implementation enhances type safety and readability without introducing unnecessary complexity.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other occurrences of uint56 in the codebase rg --type solidity "uint56" -C 2Length of output: 717
ethereum/contracts/test/WinningProbabilityOracle.t.sol (5)
4-6: LGTM: Imports are appropriate for the test file.The import statements correctly include the necessary dependencies for testing, including the Forge Standard Library, OpenZeppelin's Ownable2Step, and the custom WinningProbabilityOracle contract.
8-11: LGTM: Ownable2StepEvents contract correctly defines ownership transfer events.The
Ownable2StepEventscontract properly defines the events emitted during ownership transfer, which will be useful for testing the ownership transfer functionality.
13-20: LGTM: Test contract setup is well-structured.The
TicketWinningProbabilityOracleTestcontract is correctly set up with appropriate inheritance and initialization. ThesetUpfunction properly initializes the test environment with a deterministic owner address and a maximum winning probability value.
34-44: LGTM: Revert tests are well-implemented.The
testRevert_setSameFailsandtestRevert_setByNonOwnerFailsfunctions correctly test the revert conditions for setting the same winning probability value and attempting to set the value by a non-owner. The use ofvm.prankandvm.expectRevertis appropriate for these tests.
70-85: LGTM: Ownership transfer test is comprehensive.The
test_transferOwnershipfunction effectively tests the two-step ownership transfer process. It correctly simulates the actions of both the current and new owner, verifies the emission of appropriate events, and checks the final state of ownership. This test provides good coverage for the ownership transfer functionality.ethereum/contracts/script/DeployAll.s.sol (3)
11-11: Import statement is correct.The import of
WinProbfromWinningProbabilityOracle.solis correctly implemented.
289-298: Deployment function follows established patterns.The
_deployHoprWinningProbabilityOraclefunction correctly deploys theWinningProbabilityOraclecontract and is consistent with the existing deployment functions.
91-93: Verify the default winning probability value.Ensure that
WinProb.wrap(type(uint56).max)accurately represents a winning probability of1.0in theWinningProbabilityOraclecontract. Confirm that this value aligns with the expected maximum and is correctly interpreted within the contract's logic.Run the following script to verify that
WinProb.wrap(type(uint56).max)corresponds to a winning probability of1.0:
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- hopli/src/win_prob.rs (1 hunks)
🧰 Additional context used
📓 Learnings (1)
hopli/src/win_prob.rs (2)
Learnt from: QYuQianchen PR: hoprnet/hoprnet#6501 File: hopli/src/win_prob.rs:121-123 Timestamp: 2024-09-25T17:18:27.689Z Learning: If code is only used once, it's acceptable to keep it inline without refactoring it into a helper function.Learnt from: QYuQianchen PR: hoprnet/hoprnet#6501 File: hopli/src/win_prob.rs:149-151 Timestamp: 2024-09-25T14:34:42.518Z Learning: In the `execute_get_win_prob` function in `win_prob.rs`, the winning probability is logged at the `info` level within the function, so additional output in `async_run` is unnecessary.
Pull request was converted to draft
…th the new contract (#6501) Co-authored-by: Lukas <lukas.pohanka@inina.net>
Description
hoplicommands to interact with winning probability contractDeployment
0xC15675d4CCa538D91a91a8D3EcFBB8499C3B0470x02e1009fd222917Ee7bdfdBF8AE1e56c4ae3F6f30x7Eb8d762fe794A108e568aD2097562cc5D3A1359Closes #6493