Refactor Obscuroscan to Tenscan #1716
Conversation
WalkthroughThe overarching change involves a rebranding from "obscuroscan" to "tenscan" across various files and configurations, affecting Docker files, GitHub workflows, and Go source files. Additionally, there are adjustments to gas limits for smart contract operations, with increases to accommodate larger transactions. The changes also include improvements to test configurations and network simulation tools, such as added delays and port adjustments, to enhance stability and performance. Changes
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? TipsChat with CodeRabbit Bot (
|
otherview
force-pushed
the
pedro/refactor_tenscan_folders
branch
from
ca0bf78 to
06ecc56
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (33)
- docs/_data/navigation.yml
- testnet/docker-compose.local.yml
- tools/obscuroscan/static/favicon-32x32.png
- tools/obscuroscan/static/image-dark.png
- tools/obscuroscan/static/logo.png
- tools/obscuroscan_v2/frontend/package-lock.json
- tools/obscuroscan_v2/frontend/package.json
- tools/obscuroscan_v2/frontend/public/favicon-32x32.png
- tools/obscuroscan_v2/frontend/src/assets/imgs/github-mark-white.png
- tools/obscuroscan_v2/frontend/src/assets/imgs/github_logo.png
- tools/obscuroscan_v2/frontend/src/assets/imgs/icon_contracts.png
- tools/obscuroscan_v2/frontend/src/assets/imgs/icon_ethereum.png
- tools/obscuroscan_v2/frontend/src/assets/imgs/icon_l1_rollup.png
- tools/obscuroscan_v2/frontend/src/assets/imgs/icon_l2_batch.png
- tools/obscuroscan_v2/frontend/src/assets/imgs/icon_metamask.png
- tools/obscuroscan_v2/frontend/src/assets/imgs/icon_nodes.png
- tools/obscuroscan_v2/frontend/src/assets/imgs/icon_transactions.png
- tools/obscuroscan_v2/frontend/src/assets/imgs/ten.svg
- tools/obscuroscan_v2/frontend/src/assets/imgs/twitter_logo.png
- tools/obscuroscan_v3/frontend/public/favicon.ico
- tools/tenscan/frontend/components.json
- tools/tenscan/frontend/package-lock.json
- tools/tenscan/frontend/package.json
- tools/tenscan/frontend/public/assets/images/clock.png
- tools/tenscan/frontend/public/assets/images/ten.svg
- tools/tenscan/frontend/public/docs/privacy.json
- tools/tenscan/frontend/public/docs/terms.json
- tools/tenscan/frontend/public/favicon.ico
- tools/tenscan/frontend/public/next.svg
- tools/tenscan/frontend/public/vercel.svg
- tools/tenscan/frontend/styles/fonts/CloudSoft-Bold_700.otf
- tools/tenscan/frontend/styles/fonts/CloudSoft-Light_300.otf
- tools/tenscan/frontend/tsconfig.json
Files selected for processing (22)
- .dockerignore (1 hunks)
- .github/workflows/build-pr.yml (1 hunks)
- .github/workflows/manual-deploy-ten-scan.yml (3 hunks)
- .gitignore (2 hunks)
- docs/_docs/testnet/deploying-a-smart-contract.md (1 hunks)
- go/common/httputil/httputil.go (1 hunks)
- go/common/log/log.go (1 hunks)
- go/host/container/host_container.go (2 hunks)
- go/host/rpc/clientapi/client_api_obscuroscan.go (4 hunks)
- go/rpc/client.go (1 hunks)
- integration/obscuroscan/obscuroscan_test.go (4 hunks)
- integration/simulation/p2p/in_mem_obscuro_client.go (6 hunks)
- integration/simulation/validate_chain.go (2 hunks)
- testnet/testnet-local-build_images.sh (1 hunks)
- tools/tenscan/backend/Dockerfile (1 hunks)
- tools/tenscan/backend/cmd/cli.go (2 hunks)
- tools/tenscan/backend/cmd/main.go (2 hunks)
- tools/tenscan/backend/container/container.go (2 hunks)
- tools/tenscan/backend/webserver/webserver.go (1 hunks)
- tools/tenscan/frontend/Dockerfile (1 hunks)
- tools/tenscan/frontend/README.md (1 hunks)
- tools/tenscan/frontend/pages/index.tsx (1 hunks)
Files skipped from review due to trivial changes (9)
- .dockerignore
- .github/workflows/build-pr.yml
- docs/_docs/testnet/deploying-a-smart-contract.md
- go/common/httputil/httputil.go
- go/host/container/host_container.go
- testnet/testnet-local-build_images.sh
- tools/tenscan/backend/Dockerfile
- tools/tenscan/frontend/README.md
- tools/tenscan/frontend/pages/index.tsx
Files skipped from review as they are similar to previous changes (12)
- .github/workflows/manual-deploy-ten-scan.yml
- .gitignore
- go/common/log/log.go
- go/host/rpc/clientapi/client_api_obscuroscan.go
- go/rpc/client.go
- integration/simulation/p2p/in_mem_obscuro_client.go
- integration/simulation/validate_chain.go
- tools/tenscan/backend/cmd/cli.go
- tools/tenscan/backend/cmd/main.go
- tools/tenscan/backend/container/container.go
- tools/tenscan/backend/webserver/webserver.go
- tools/tenscan/frontend/Dockerfile
Additional comments: 7
integration/obscuroscan/obscuroscan_test.go (7)
13-14: The imports have been updated to reflect the new paths after the renaming from "obscuroscan" to "tenscan". Ensure that the new paths are correct and the imported packages exist at the new location.
40-40: The
TestTypehas been correctly updated to "tenscan" in theinitfunction. This change aligns with the renaming objective of the PR.47-47: The constant
testLogshas been updated to point to the new tenscan build directory. Verify that this directory is correctly set up in the build process to store logs.50-58: The test function has been renamed to
TestTenscan, and the configuration objecttenScanConfighas been correctly initialized with the new service addresses. Ensure that the service is indeed running on the specified ports.61-61: The container initialization now uses
NewTenScanContainerfrom thetenscan/backend/containerpackage. Confirm that theNewTenScanContainerfunction exists and operates as expected.64-64: The call to
tenScanContainer.Start()is appropriate for starting the container. Ensure that theStartmethod is implemented correctly in theTenScanContainertype.186-186: The container is being stopped with
tenScanContainer.Stop(). Verify that theStopmethod properly terminates the container without leaving any resources hanging.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- .github/workflows/build-pr.yml (1 hunks)
- integration/contractdeployer/contract_deployer_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- .github/workflows/build-pr.yml
Additional comments: 1
integration/contractdeployer/contract_deployer_test.go (1)
- 38-44: The change in the
initfunction'sTestTypefrom "noderunner" to "contractdeployer" is consistent with the renaming effort described in the PR. This change should be verified to ensure that it aligns with the intended test configuration and does not affect other tests that may rely on the previousTestType.
| // todo remove this sleep when tests stabilize | ||
| time.Sleep(10 * time.Second) |
There was a problem hiding this comment.
The introduction of a time.Sleep call in tests is generally a sign of potential flakiness or synchronization issues. It's better to use a more deterministic synchronization mechanism, such as waiting for a specific condition or event.
- // todo remove this sleep when tests stabilize
- time.Sleep(10 * time.Second)
+ // TODO: Implement a more robust synchronization mechanismCommittable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| // todo remove this sleep when tests stabilize | |
| time.Sleep(10 * time.Second) | |
| // TODO: Implement a more robust synchronization mechanism |
| // todo remove this sleep when tests stabilize | ||
| time.Sleep(20 * time.Second) | ||
| err = tenGwContainer.Stop() | ||
| assert.NoError(t, err) |
There was a problem hiding this comment.
The introduction of a fixed time.Sleep delay can lead to brittle tests that may either wait unnecessarily long or not long enough depending on system load and other factors. Consider using a more dynamic wait strategy, such as polling for a condition or using synchronization primitives.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (14)
- .github/workflows/build-pr.yml (1 hunks)
- contracts/deployment_scripts/testnet/layer2/001_whitelist_tokens.ts (1 hunks)
- go/config/enclave_cli_flags.go (1 hunks)
- go/enclave/crosschain/message_bus_manager.go (2 hunks)
- go/enclave/enclave.go (1 hunks)
- go/enclave/nodetype/sequencer.go (1 hunks)
- integration/common/constants.go (1 hunks)
- integration/common/utils.go (1 hunks)
- integration/constants.go (2 hunks)
- integration/eth2network/eth2_network.go (4 hunks)
- integration/simulation/network/socket.go (3 hunks)
- integration/simulation/simulation.go (1 hunks)
- integration/smartcontract/smartcontracts_test.go (1 hunks)
- tools/hardhatdeployer/contract_deployer.go (1 hunks)
Files skipped from review due to trivial changes (1)
- .github/workflows/build-pr.yml
Additional comments: 17
integration/constants.go (2)
- 5-16: Verify that the new port numbers do not conflict with other services and are within the acceptable range for the system.
Verification successful
The verification script attempted to use
lsofto check for port conflicts, butlsofwas not found in the environment. Therefore, the script's output is not reliable for determining if there are port conflicts. An alternative method should be used to verify port availability.
The verification script using
netcathas been executed and the output confirms that there are no conflicts on the ports checked (10000, 14000, 18000, 22000, 26000, 30000, 34000, 38000, 42000, 48000, 52000, 56000). This suggests that the new port numbers assigned in theintegration/constants.gofile do not conflict with other services on the system and are within an acceptable range.* 27-29: Ensure that the new default port offsets are not causing any conflicts or issues with the existing network configuration.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential port conflicts on the system for port in {10000,14000,18000,22000,26000,30000,34000,38000,42000,48000,52000,56000}; do echo "Checking port $port" if lsof -i:$port; then echo "Conflict found on port $port" else echo "No conflict on port $port" fi doneLength of output: 3431
Script:
#!/bin/bash # Alternative check for potential port conflicts on the system using netcat for port in {10000,14000,18000,22000,26000,30000,34000,38000,42000,48000,52000,56000}; do echo "Checking port $port" if nc -zv 127.0.0.1 $port 2>&1 | grep -q 'succeeded'; then echo "Conflict found on port $port" else echo "No conflict on port $port" fi doneLength of output: 2915
integration/common/constants.go (1)
- 86-87: Confirm that the new gas limits are justified and do not negatively impact block processing times or network throughput.
integration/common/utils.go (1)
- 102-102: Ensure that the reduced gas value is sufficient for the
PrefundWalletstransactions to complete successfully.contracts/deployment_scripts/testnet/layer2/001_whitelist_tokens.ts (1)
- 125-125: Check that the increased gas limit for the
relayMessagefunction is appropriate and does not lead to excessive gas costs.go/config/enclave_cli_flags.go (2)
58-58: Confirm that the updated default value and description for
GasBatchExecutionLimitare clear and accurately reflect the intended usage.65-65: Ensure that the updated default value and description for
GasLocalExecutionCapFlagare clear and accurately reflect the intended usage.tools/hardhatdeployer/contract_deployer.go (1)
- 95-95: Check that the increased gas limit for contract deployment is appropriate and does not lead to unnecessary gas expenditure.
go/enclave/crosschain/message_bus_manager.go (2)
86-92: The gas limit for the
GenerateMessageBusDeployTxfunction has been increased from 2,000,000 to 5,000,000. Verify that this increase is necessary and justified by the complexity of the contract being deployed, and consider the impact on transaction costs and network performance.219-225: Similarly, the gas limit for the
CreateSyntheticTransactionsfunction has been increased to 5,000,000. Ensure that this increase is required for the transactions being created and assess the potential impact on the system.integration/simulation/simulation.go (1)
- 227-233: The gas limit for deploying ERC20 contracts on Obscuro in the
deployObscuroERC20smethod has been increased to 5,000,000. Confirm that this increase is necessary for the contract deployment and consider the impact on transaction costs and network performance.integration/smartcontract/smartcontracts_test.go (1)
- 41-41: The change in
TestTypefrom "noderunner" to "smartcontracts" within theinitfunction is a configuration update that seems appropriate for the context of the tests in this file.integration/eth2network/eth2_network.go (5)
22-22: The addition of
gethcommonimport is noted. Ensure that this import is used within the file and is not an unnecessary addition.211-213: The
Startmethod now includes a check for duplicated networks usingensureNoDuplicatedNetwork. Ensure that this method is robust and correctly prevents network duplication without false positives.516-518: The
Startmethod now includes a call toprefundedBalancesActiveto check prefunded account balances. Ensure that this method correctly verifies the balances and handles any errors appropriately.603-616: The
prefundedBalancesActivemethod has been added to check and confirm the prefunded account balances. Ensure that the logic correctly checks the balances and that the error messages are clear and informative.618-626: The
ensureNoDuplicatedNetworkmethod has been added to prevent duplicated networks during initialization. Verify that the logic correctly identifies active nodes and that the error handling is appropriate.
go/enclave/enclave.go
Outdated
| headBatch := e.registry.HeadBatchSeq() | ||
| if headBatch != nil { | ||
| l2Head, err := e.storage.FetchBatchBySeqNo(headBatch.Uint64()) | ||
| if err == nil { | ||
| // todo - we should return an error when head state is not available, but for current test situations with race | ||
| // conditions we allow it to return zero while head state is uninitialized | ||
| s, err := e.storage.CreateStateDB(l2Head.Hash()) | ||
| if err != nil { | ||
| return nil, responses.ToInternalError(err) | ||
| } | ||
| nonce = s.GetNonce(address) | ||
| } | ||
| nonce = s.GetNonce(address) | ||
| } |
There was a problem hiding this comment.
The refactoring of nonce retrieval logic to handle situations where the head batch is not available is a good improvement. However, the comment on lines 692-693 suggests that there may be test scenarios with race conditions that are not fully addressed. It would be beneficial to ensure that these race conditions are handled properly in production code to avoid potential issues.
Additionally, the error handling on line 696 is correct, but it would be helpful to log the error for debugging purposes. It's important to have visibility into why creating the state DB failed, especially since this operation is critical for nonce retrieval.
if err != nil {
+ e.logger.Error("Failed to create state DB", log.ErrKey, err, log.BatchHashKey, l2Head.Hash())
return nil, responses.ToInternalError(err)
}Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| headBatch := e.registry.HeadBatchSeq() | |
| if headBatch != nil { | |
| l2Head, err := e.storage.FetchBatchBySeqNo(headBatch.Uint64()) | |
| if err == nil { | |
| // todo - we should return an error when head state is not available, but for current test situations with race | |
| // conditions we allow it to return zero while head state is uninitialized | |
| s, err := e.storage.CreateStateDB(l2Head.Hash()) | |
| if err != nil { | |
| return nil, responses.ToInternalError(err) | |
| } | |
| nonce = s.GetNonce(address) | |
| } | |
| nonce = s.GetNonce(address) | |
| } | |
| headBatch := e.registry.HeadBatchSeq() | |
| if headBatch != nil { | |
| l2Head, err := e.storage.FetchBatchBySeqNo(headBatch.Uint64()) | |
| if err == nil { | |
| // todo - we should return an error when head state is not available, but for current test situations with race | |
| // conditions we allow it to return zero while head state is uninitialized | |
| s, err := e.storage.CreateStateDB(l2Head.Hash()) | |
| if err != nil { | |
| e.logger.Error("Failed to create state DB", log.ErrKey, err, log.BatchHashKey, l2Head.Hash()) | |
| return nil, responses.ToInternalError(err) | |
| } | |
| nonce = s.GetNonce(address) | |
| } | |
| } |
go/enclave/nodetype/sequencer.go
Outdated
| @@ -163,6 +163,7 @@ func (s *sequencer) createGenesisBatch(block *common.L1Block) error { | |||
| return err | |||
| } | |||
|
|
|||
| time.Sleep(time.Second) | |||
There was a problem hiding this comment.
The introduction of time.Sleep(time.Second) could lead to potential timing issues or inefficiencies. If this delay is for synchronization purposes, consider using a more robust mechanism such as condition variables or channels to handle the synchronization more effectively.
| // getProcessesUsingPort returns a slice of process details using the specified port. | ||
| func checkProcessPort(errPort string) error { | ||
| re := regexp.MustCompile(`:(\d+):`) | ||
| matches := re.FindStringSubmatch(errPort) | ||
|
|
||
| if len(matches) < 2 { | ||
| return fmt.Errorf("no port found in string") | ||
| } | ||
|
|
||
| port := matches[1] | ||
|
|
||
| cmd := exec.Command("lsof", "-i", fmt.Sprintf(":%s", port)) //nolint:gosec | ||
|
|
||
| output, err := cmd.Output() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| var processes []string | ||
| scanner := bufio.NewScanner(strings.NewReader(string(output))) | ||
| for scanner.Scan() { | ||
| line := scanner.Text() | ||
| if strings.Contains(line, "LISTEN") || strings.Contains(line, "ESTABLISHED") { | ||
| processes = append(processes, line) | ||
| } | ||
| } | ||
|
|
||
| fmt.Printf("Found processes still opened on port %s - %+v\n", port, processes) | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
The function checkProcessPort directly prints the processes using a port to stdout. Consider refactoring it to return the processes so the caller can decide how to handle the output. Additionally, add error handling for the scanner to handle potential scanning errors.
- fmt.Printf("Found processes still opened on port %s - %+v\n", port, processes)
+ return processes, nilAlso, check for errors after the scanning loop:
if err := scanner.Err(); err != nil {
return nil, fmt.Errorf("scanner error: %v", err)
}
otherview
temporarily deployed
to
uat-testnet
GitHub Actions
Inactive
otherview
temporarily deployed
to
sepolia-testnet
GitHub Actions
Inactive
otherview
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
|
Deployed and Updated dev, uat and sepolia deployments 👍 |
Why this change is needed
https://github.com/ten-protocol/ten-internal/issues/2578
What changes were made as part of this PR
Please provide a high level list of the changes made
PR checks pre-merging
Please indicate below by ticking the checkbox that you have read and performed the required
PR checks