add token as query param #1666
add token as query param #1666
Conversation
WalkthroughThe changes across the codebase reflect a significant rebranding effort, shifting from "Obscuro" to "Ten" as the primary identifier. This includes updates to chain IDs, variable names, function calls, and URL parameters. A notable shift is the replacement of user IDs with encryption tokens for authentication purposes. The updates are consistent and thorough, affecting test configurations, library functions, network setups, and documentation to align with the new naming convention and authentication strategy. Changes
TipsChat with CodeRabbit Bot (
|
![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: 28
Configuration used: CodeRabbit UI
Files selected for processing (25)
- design/ux/Ten_Gateway.md (12 hunks)
- integration/constants.go (1 hunks)
- integration/contractdeployer/contract_deployer_test.go (5 hunks)
- integration/faucet/faucet_test.go (3 hunks)
- integration/manualtests/connection_test.go (1 hunks)
- integration/networktest/actions/setup_actions.go (1 hunks)
- integration/networktest/env/testnet.go (1 hunks)
- integration/networktest/userwallet/userwallet.go (2 hunks)
- integration/obscurogateway/gateway_user.go (2 hunks)
- integration/obscurogateway/tengateway_test.go (16 hunks)
- integration/obscuroscan/obscuroscan_test.go (3 hunks)
- integration/simulation/devnetwork/config.go (2 hunks)
- integration/simulation/devnetwork/dev_network.go (1 hunks)
- integration/simulation/devnetwork/node.go (2 hunks)
- integration/simulation/network/network_utils.go (1 hunks)
- integration/simulation/simulation_full_network_test.go (1 hunks)
- integration/simulation/simulation_geth_in_mem_test.go (1 hunks)
- integration/simulation/simulation_in_mem_test.go (1 hunks)
- tools/gateway-js/gateway-lib/gateway.js (2 hunks)
- tools/walletextension/README.md (1 hunks)
- tools/walletextension/api/staticOG/javascript.js (4 hunks)
- tools/walletextension/api/utils.go (1 hunks)
- tools/walletextension/common/constants.go (1 hunks)
- tools/walletextension/lib/client_lib.go (4 hunks)
- tools/walletextension/test/utils.go (1 hunks)
Files skipped from review due to trivial changes (1)
- tools/walletextension/api/staticOG/javascript.js
Additional comments: 56
design/ux/Ten_Gateway.md (2)
306-306: Consider elaborating on the privacy and security implications of allowing other addresses to view certain data.
317-318: Publishing revocations on a ledger is a good practice for transparency and could prevent abuse by the TG operator. Ensure that this mechanism is implemented securely and efficiently.
integration/constants.go (1)
- 12-35: The renaming of constants from
ObscurotoTenis consistent with the summary and the pull request details. Ensure that all references to these constants throughout the codebase have been updated to reflect the new names.integration/contractdeployer/contract_deployer_test.go (2)
56-62: The configuration for deploying the Layer2ERC20Contract has been updated to use the
TenChainID. Ensure that theChainIDis correctly set in all environments where this test will run.106-112: The configuration for deploying the Layer2ERC20Contract within the
TestFaucetSendsFundsOnlyIfNeededtest has been updated to use theTenChainID. Verify that theChainIDis correctly set in all environments where this test will run.integration/faucet/faucet_test.go (2)
52-57: The update of the
ChainIDfromintegration.ObscuroChainIDtointegration.TenChainIDin the faucet configuration is consistent with the overall changes in the pull request.69-71: The use of
integration.TenChainIDin theRandomWalletfunction call aligns with the changes described in the summary and pull request.integration/manualtests/connection_test.go (1)
- 22-28: The WebSocket address
ogWSAddressis still using theobscu.rodomain. Verify if this is intentional or if it should be updated to reflect the transition to the Ten chain.integration/networktest/actions/setup_actions.go (2)
24-24: The update from
integration.ObscuroChainIDtointegration.TenChainIDaligns with the overall transition from Obscuro to Ten chain as described in the summary.22-27: Verify that all instances where
integration.ObscuroChainIDwas used have been updated tointegration.TenChainIDacross the entire codebase to maintain consistency.integration/networktest/env/testnet.go (2)
52-62: The changes to the
testnetConnectorstruct and methods to support the Ten chain appear to be correct. Ensure that all other parts of the codebase that interact with this struct or its methods are updated accordingly to handle the new chain ID and any other related changes.64-64: Ensure that the
AllocateFaucetFundsmethod is updated to be consistent with the new Ten chain ID and any other related changes, even though the actual implementation is not shown here.integration/networktest/userwallet/userwallet.go (2)
70-73: The default chain ID has been updated to
TenChainID. Ensure that all relevant parts of the system that depend on the chain ID are aware of this change to avoid any inconsistencies.84-85: The
ChainIDfunction has been updated to returnTenChainID. Verify that this change is reflected wherever theChainIDfunction is used to ensure compatibility with the new chain.integration/obscurogateway/gateway_user.go (3)
19-19: The change from
lib.OGLibtolib.TGLibfor theogClientfield in theGatewayUserstruct is consistent with the transition from Obscuro to Ten chain. Ensure that all methods and functionalities oflib.TGLibare compatible with the existing code that usesogClient.23-23: The initialization of
ogClientwithlib.NewTenGatewayLibraryaligns with the transition to the Ten chain. Verify thatlib.NewTenGatewayLibraryis properly implemented and tested to ensure it works as expected in the context ofNewUser.32-32: The update to append
"?token=" + ogClient.UserID()to the server addresses for both HTTP and WebSocket clients is consistent with the new token-based user identification mechanism. Ensure that the server endpoints are expecting this new query parameter and that theUserIDmethod ofogClientis correctly retrieving the user's token.Also applies to: 36-36
integration/obscurogateway/tengateway_test.go (8)
40-51: The renaming of the test type and related constants appears to be consistent and correctly updated.
40-47: The use of the
initfunction is acknowledged with a directive to suppress lint warnings. Ensure that the use ofinitis necessary and that the code within it cannot be initialized in a more predictable manner elsewhere.54-55: The
createTenNetworkfunction is called without error handling in the test function, but it handles errors internally by panicking. Ensure that this behavior is acceptable for the test setup.540-550: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [529-548]
The
createTenNetworkfunction uses a panic to handle errors during network creation. This is an acceptable way to fail fast in test setup if the environment cannot be established.
106-107: The
tenGwContainer.Stop()function is called with error checking usingassert.NoError, which is appropriate for test code to ensure graceful shutdown.482-484: The re-establishment of the websocket connection uses a hardcoded path and query parameter. Verify that this is correct and consider making it configurable if necessary.
525-526: The
transferRandomAddrfunction is marked as unused withnolint: unused. Ensure that this function is indeed used elsewhere in the test suite and that the directive is appropriate.438-439: The
subscribeToEventsfunction has anolint:unparamdirective. Verify that the function's design is intentionally generic and that the parameters are indeed used in some cases.integration/obscuroscan/obscuroscan_test.go (12)
206-206: Verify if the
obsclient.NewAuthObsClientfunction has been renamed to align with the Tenscan naming convention and update the test code accordingly.206-206: The
integration.EthereumChainIDconstant is used here. Verify that this constant is still relevant after the transition to Tenscan and update if necessary.206-206: The
integration.TenChainIDconstant is correctly used here, reflecting the new chain ID after the transition to Tenscan.208-209: The
AvgBlockDurationis set to1 * time.Second. Ensure that this value is appropriate for the new Tenscan network's expected block time.208-209: The
MgmtContractLibandERC20ContractLibare mocked here. Confirm that these mocks are still valid and reflect any new logic or interfaces introduced with the Tenscan transition.73-77: The
issueTransactionsfunction is called withintegration.TenChainID, which is correct and reflects the new chain ID after the transition to Tenscan.73-77: The
issueTransactionsfunction is called withgenesis.TestnetPrefundedPK. Verify that this private key is still relevant and has the necessary funds in the new Tenscan testnet.50-57: The configuration for
obsScanConfighas been updated with the new server addresses and ports. Confirm that these values are correct for the Tenscan environment.50-57: The
LogPathis set to"sys_out". Ensure that this is the intended log path for the Tenscan environment and update if necessary.58-58: The
serverAddressvariable is constructed using the updatedobsScanConfig.ServerAddress. Confirm that this address is correct for the Tenscan environment.58-58: The
serverAddressis used to construct URLs for HTTP requests. Verify that the endpoints being called are updated to reflect the new Tenscan naming convention and functionality.58-58: The endpoint
/info/obscuro/might need to be updated to/info/tenscan/or a similar endpoint that reflects the new Tenscan naming convention.integration/simulation/devnetwork/config.go (3)
40-41: The update from
ObscuroChainIDtoTenChainIDin theDefaultDevNetworkfunction aligns with the overall transition from the Obscuro chain to the Ten chain.74-74: The update from
ObscuroChainIDtoTenChainIDin theLiveL1DevNetworkfunction aligns with the overall transition from the Obscuro chain to the Ten chain.38-44: The
ObscuroConfigstruct is still being used despite the transition to the Ten chain. Please verify if this is intentional for backward compatibility or if it should be updated or removed.integration/simulation/devnetwork/dev_network.go (1)
- 61-62: The change from
ObscuroChainIDtoTenChainIDis consistent with the pull request's intention to transition from using the Obscuro chain to the Ten chain.integration/simulation/devnetwork/node.go (3)
125-127: The
L1StartHashfield is still usingn.l1Data.ObscuroStartBlock. If this field is meant to represent the starting block for the Ten chain, it should be renamed to reflect the new chain's context.123-129: The update of
ObscuroChainIDtointegration.TenChainIDis consistent with the overall changes in the pull request. Ensure that all references to the Obscuro chain are updated across the entire codebase.167-173: The update of
ObscuroChainIDtointegration.TenChainIDin thecreateEnclaveContainerfunction is consistent with the changes in thecreateHostContainerfunction and the overall pull request.integration/simulation/network/network_utils.go (2)
86-86: The change from
ObscuroChainIDtoTenChainIDaligns with the overall pull request's goal of transitioning from Obscuro to Ten chain components.83-89: Ensure that all references to the chain ID throughout the codebase have been updated to use
TenChainIDinstead ofObscuroChainIDto maintain consistency.integration/simulation/simulation_full_network_test.go (1)
- 23-23: The update of the chain ID in the
walletsinitialization fromintegration.ObscuroChainIDtointegration.TenChainIDis consistent with the overall transition from the Obscuro chain to the Ten chain as described in the pull request overview. Ensure that all related configurations and tests are updated accordingly.integration/simulation/simulation_geth_in_mem_test.go (1)
- 26-26: The change from
integration.ObscuroChainIDtointegration.TenChainIDis consistent with the overall transition from the Obscuro chain to the Ten chain. Ensure that all dependent configurations, network behaviors, and test assertions are updated to align with the new chain ID.integration/simulation/simulation_in_mem_test.go (1)
- 21-27: The change from
integration.ObscuroChainIDtointegration.TenChainIDis consistent with the overall pull request's goal of transitioning from the Obscuro chain to the Ten chain. Ensure that this change is reflected in all parts of the codebase where the chain ID is relevant.tools/walletextension/README.md (1)
- 38-38: The documentation link provided at the top of the README might need to be updated to reflect the new Ten chain documentation if it has changed.
tools/walletextension/common/constants.go (4)
41-41: The addition of
EncryptedTokenQueryParameteris consistent with the pull request's transition to using an encryption token for user identification.38-47: Ensure that the removal of previously used constants does not affect any existing functionality that may still rely on them. Verify that all references to these constants have been removed or updated throughout the codebase.
40-40: Verify whether the presence of the old
UserQueryParameteris intentional and ensure it does not conflict with the newEncryptedTokenQueryParameter.38-47: The renaming of components related to the Obscuro chain to the Ten chain, as mentioned in the pull request summary, is not reflected in this hunk. Ensure that all necessary renamings are consistently applied throughout the codebase.
tools/walletextension/lib/client_lib.go (1)
- 49-49: The
RegisterAccountmethod usesintegration.TenChainIDdirectly. Ensure that this constant is correctly defined in theintegrationpackage to match the new Ten chain ID.tools/walletextension/test/utils.go (1)
- 126-130: The change from "u" to "token" in the query parameter is consistent with the system-wide update to use an encryption token for user identification. Ensure that all parts of the system that interact with this function are updated to use the new "token" parameter.
| func (s *UserWallet) ChainID() *big.Int { | ||
| return big.NewInt(integration.ObscuroChainID) | ||
| return big.NewInt(integration.TenChainID) |
There was a problem hiding this comment.
Consider returning the chainID field directly from the ChainID function to avoid creating a new big.Int instance on each call.
func (s *UserWallet) ChainID() *big.Int {
- return big.NewInt(integration.TenChainID)
+ return s.chainID
}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.
| func (s *UserWallet) ChainID() *big.Int { | |
| return big.NewInt(integration.ObscuroChainID) | |
| return big.NewInt(integration.TenChainID) | |
| func (s *UserWallet) ChainID() *big.Int { | |
| return s.chainID | |
| } |
| It generates and returns userID which needs to be added as a query parameter "u" to the URL in your Metamask | ||
| (or another provider) as it identifies you. |
There was a problem hiding this comment.
The description still mentions the old query parameter "u" which is inconsistent with the new changes to use "token". This should be updated to avoid confusion.
- It generates and returns userID which needs to be added as a query parameter "u" to the URL in your Metamask
+ It generates and returns userID which needs to be added as a query parameter "token" to the URL in your MetamaskCommittable 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.
| It generates and returns userID which needs to be added as a query parameter "u" to the URL in your Metamask | |
| (or another provider) as it identifies you. | |
| It generates and returns userID which needs to be added as a query parameter "token" to the URL in your Metamask | |
| (or another provider) as it identifies you. |
| userID, err := getQueryParameter(conn.ReadRequestParams(), common.EncryptedTokenQueryParameter) | ||
| if err == nil { | ||
| if len(userID) != common.MessageUserIDLen { | ||
| return "", fmt.Errorf(fmt.Sprintf("wrong length of userID from URL. Got: %d, Expected: %d", len(userID), common.MessageUserIDLen)) |
There was a problem hiding this comment.
The error message construction is redundant. Simplify by removing fmt.Sprintf and directly using fmt.Errorf with formatting directives.
- return "", fmt.Errorf(fmt.Sprintf("wrong length of userID from URL. Got: %d, Expected: %d", len(userID), common.MessageUserIDLen))
+ return "", fmt.Errorf("wrong length of userID from URL. Got: %d, Expected: %d", len(userID), common.MessageUserIDLen)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.
| return "", fmt.Errorf(fmt.Sprintf("wrong length of userID from URL. Got: %d, Expected: %d", len(userID), common.MessageUserIDLen)) | |
| return "", fmt.Errorf("wrong length of userID from URL. Got: %d, Expected: %d", len(userID), common.MessageUserIDLen) |
| } | ||
|
|
||
| // try getting userID(`u`) from query parameters and return it if successful | ||
| userID, err = getQueryParameter(conn.ReadRequestParams(), common.UserQueryParameter) | ||
| if err == nil { | ||
| if len(userID) != common.MessageUserIDLen { | ||
| return "", fmt.Errorf(fmt.Sprintf("wrong length of userID from URL. Got: %d, Expected: %d", len(userID), common.MessageUserIDLen)) |
There was a problem hiding this comment.
The same issue as above. Simplify the error message construction.
- return "", fmt.Errorf(fmt.Sprintf("wrong length of userID from URL. Got: %d, Expected: %d", len(userID), common.MessageUserIDLen))
+ return "", fmt.Errorf("wrong length of userID from URL. Got: %d, Expected: %d", len(userID), common.MessageUserIDLen)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.
| return "", fmt.Errorf(fmt.Sprintf("wrong length of userID from URL. Got: %d, Expected: %d", len(userID), common.MessageUserIDLen)) | |
| return "", fmt.Errorf("wrong length of userID from URL. Got: %d, Expected: %d", len(userID), common.MessageUserIDLen) |
|
|
||
| With this endpoint, you submit a signed message in the format `Register <userID> for <account>` | ||
| from that account which proves that you hold private keys for it, and it links that account with your userID. | ||
|
|
||
| - `GET /v1/query/address?u=$UserId&a=$Address` | ||
| - `GET /v1/query/address?token=$EncryptionToken&a=$Address` | ||
|
|
||
| This endpoint responds with a JSON of true or false if the address "a" is already registered for user "u" |
There was a problem hiding this comment.
The endpoint description incorrectly references the old "u" parameter instead of the new "token" parameter. This should be corrected to match the updated endpoint.
- This endpoint responds with a JSON of true or false if the address "a" is already registered for user "u"
+ This endpoint responds with a JSON of true or false if the address "a" is already registered for user "token"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.
| This endpoint responds with a JSON of true or false if the address "a" is already registered for user "u" | |
| This endpoint responds with a JSON of true or false if the address "a" is already registered for user "token" |
| @@ -44,7 +44,7 @@ class Gateway { | |||
| } | |||
|
|
|||
| try { | |||
| const authenticateUserURL = pathAuthenticate+"?u="+this.userId | |||
| const authenticateUserURL = pathAuthenticate+"?token="+this.userId | |||
There was a problem hiding this comment.
The authenticateUserURL is missing the base URL (this.httpURL) when concatenated, which will result in an incorrect URL.
- const authenticateUserURL = pathAuthenticate+"?token="+this.userId
+ const authenticateUserURL = `${this.httpURL}${pathAuthenticate}?token=${this.userId}`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.
| const authenticateUserURL = pathAuthenticate+"?token="+this.userId | |
| const authenticateUserURL = `${this.httpURL}${pathAuthenticate}?token=${this.userId}` |
There is a typo in the variable name this.userID should be this.userId.
- const message = `Register ${this.userID} for ${address.toLowerCase()}`;
+ const message = `Register ${this.userId} for ${address.toLowerCase()}`;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.
| const authenticateUserURL = pathAuthenticate+"?token="+this.userId | |
| const authenticateUserURL = pathAuthenticate+"?token="+this.userId |
| # The Ten Gateway - Design | ||
|
|
||
| The scope of this document is to design a hosted [Wallet Extension](wallet_extension.md) called the "Ten Gateway" (OG). | ||
| The scope of this document is to design a hosted [Wallet Extension](wallet_extension.md) called the "Ten Gateway" (TG). | ||
|
|
||
| The OG will be a superset of the WE functionality, so this document will only cover the additions. | ||
| The TG will be a superset of the WE functionality, so this document will only cover the additions. | ||
|
|
||
| ## High level overview | ||
|
|
||
| The OG will be a [Confidential Web Service](https://medium.com/p/983a2a67fc08), running inside SGX. | ||
| The TG will be a [Confidential Web Service](https://medium.com/p/983a2a67fc08), running inside SGX. | ||
|
|
||
| The current WE is designed to be used by a single user holding multiple addresses accross potentially multiple wallets. | ||
|
|
||
| The OG must support mutiple users, each with multiple addresses. It can be seen as offering a WE per user. | ||
| The TG must support mutiple users, each with multiple addresses. It can be seen as offering a WE per user. | ||
|
|
||
| The Ten node has no concept of "User". It only authenticates based on the "blockchain address". | ||
| It expects to be supplied with a signed viewing key per address, so that it can respond encrypted with that VK. |
There was a problem hiding this comment.
Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [77-77]
The UX diagram filename "og_ux.png" should be updated to match the new project naming convention, such as "tg_ux.png".
| @@ -44,7 +44,7 @@ | |||
| } | |||
|
|
|||
| try { | |||
| const authenticateUserURL = pathAuthenticate+"?u="+this.userId | |||
| const authenticateUserURL = pathAuthenticate+"?token="+this.userId | |||
| const authenticateFields = {"signature": signature, "message": message} | |||
| const authenticateResp = await axios.post( | |||
| authenticateUserURL, | |||
There was a problem hiding this comment.
The registerAccount function should await the authenticateResp.text() call to return the actual text instead of a promise.
- return await authenticateResp.text()
+ return (await authenticateResp.text())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.
| authenticateUserURL, | |
| authenticateUserURL, |
| statusCode, userID, err := fasthttp.Get(nil, fmt.Sprintf("%s/v1/join/", o.httpURL)) | ||
| if err != nil || statusCode != 200 { |
There was a problem hiding this comment.
In the Join method, the error message construction uses fmt.Sprintf inside another fmt.Errorf, which is redundant. You can directly use fmt.Errorf with formatting directives.
- return fmt.Errorf(fmt.Sprintf("Failed to get userID. Status code: %d, err: %s", statusCode, err))
+ return fmt.Errorf("Failed to get userID. Status code: %d, err: %v", statusCode, 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.
| statusCode, userID, err := fasthttp.Get(nil, fmt.Sprintf("%s/v1/join/", o.httpURL)) | |
| if err != nil || statusCode != 200 { |
| @@ -64,7 +64,7 @@ | |||
| req, err := http.NewRequestWithContext( | |||
| context.Background(), | |||
| http.MethodPost, | |||
| o.httpURL+"/v1/authenticate/?u="+string(o.userID), | |||
| o.httpURL+"/v1/authenticate/?token="+string(o.userID), | |||
There was a problem hiding this comment.
In the RegisterAccount method, the URL construction for the HTTP request is done by concatenating strings. Consider using a URL builder or proper query parameter encoding to avoid potential issues with special characters in the userID.
- o.httpURL+"/v1/authenticate/?token="+string(o.userID)
+ url := fmt.Sprintf("%s/v1/authenticate/", o.httpURL)
+ q := url.Values{}
+ q.Set("token", string(o.userID))
+ urlWithParams := url + "?" + q.Encode()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.
| o.httpURL+"/v1/authenticate/?token="+string(o.userID), | |
| url := fmt.Sprintf("%s/v1/authenticate/", o.httpURL) | |
| q := url.Values{} | |
| q.Set("token", string(o.userID)) | |
| urlWithParams := url + "?" + q.Encode(), |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 12
Configuration used: CodeRabbit UI
Files selected for processing (2)
- integration/obscurogateway/gateway_user.go (3 hunks)
- integration/obscurogateway/tengateway_test.go (15 hunks)
Additional comments: 1
integration/obscurogateway/gateway_user.go (1)
- 49-58: The
RegisterAccountsmethod has been correctly updated to use thetgClient.
zkokelj
force-pushed
the
ziga/add_token_as_query_param
branch
from
2185d9d to
007a718
Compare
zkokelj
force-pushed
the
ziga/add_token_as_query_param
branch
from
007a718 to
4cdec6a
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 24
Configuration used: CodeRabbit UI
Files selected for processing (25)
- design/ux/Ten_Gateway.md (12 hunks)
- integration/constants.go (1 hunks)
- integration/contractdeployer/contract_deployer_test.go (5 hunks)
- integration/faucet/faucet_test.go (3 hunks)
- integration/manualtests/connection_test.go (1 hunks)
- integration/networktest/actions/setup_actions.go (1 hunks)
- integration/networktest/env/testnet.go (1 hunks)
- integration/networktest/userwallet/userwallet.go (2 hunks)
- integration/obscurogateway/gateway_user.go (3 hunks)
- integration/obscurogateway/tengateway_test.go (15 hunks)
- integration/obscuroscan/obscuroscan_test.go (3 hunks)
- integration/simulation/devnetwork/config.go (2 hunks)
- integration/simulation/devnetwork/dev_network.go (1 hunks)
- integration/simulation/devnetwork/node.go (2 hunks)
- integration/simulation/network/network_utils.go (1 hunks)
- integration/simulation/simulation_full_network_test.go (1 hunks)
- integration/simulation/simulation_geth_in_mem_test.go (1 hunks)
- integration/simulation/simulation_in_mem_test.go (1 hunks)
- tools/gateway-js/gateway-lib/gateway.js (2 hunks)
- tools/walletextension/README.md (1 hunks)
- tools/walletextension/api/staticOG/javascript.js (4 hunks)
- tools/walletextension/api/utils.go (1 hunks)
- tools/walletextension/common/constants.go (1 hunks)
- tools/walletextension/lib/client_lib.go (4 hunks)
- tools/walletextension/test/utils.go (1 hunks)
Files skipped from review due to trivial changes (3)
- integration/networktest/userwallet/userwallet.go
- tools/walletextension/api/staticOG/javascript.js
- tools/walletextension/test/utils.go
Additional comments: 27
integration/constants.go (2)
12-30: The changes to the port offsets and the renaming of the constants are consistent with the rebranding effort and do not appear to introduce any conflicts with existing port offsets. Ensure that all references to these constants throughout the codebase have been updated accordingly.
33-34: The update of the chain ID constant from
ObscuroChainIDtoTenChainIDis in line with the rebranding. Verify that this new chain ID does not conflict with any existing chain IDs in use and that all references to the chain ID in the codebase have been updated.integration/contractdeployer/contract_deployer_test.go (5)
56-62: The update of
ChainIDfromObscuroChainIDtoTenChainIDin the contract deployment configuration is consistent with the summary and the broader changes in the codebase.69-71: The wallet and client initialization for the contract deployer now correctly uses
integration.TenChainID, aligning with the updated chain ID across the codebase.88-94: The wallet initialization for the faucet and the contract deployer within the
TestFaucetSendsFundsOnlyIfNeededtest function has been updated to useintegration.TenChainID, which is consistent with the changes described in the summary.106-112: The contract deployment configuration within the
TestFaucetSendsFundsOnlyIfNeededtest function has been updated to useintegration.TenChainID, which is in line with the changes to the chain ID across the codebase.136-136: The
walletsinitialization in thecreateObscuroNetworkfunction has been updated to useintegration.TenChainIDinstead ofintegration.ObscuroChainID. This change is consistent with the summary and the broader changes in the codebase.integration/faucet/faucet_test.go (3)
55-55: The update of
ChainIDfromintegration.ObscuroChainIDtointegration.TenChainIDis consistent with the changes described in the summary and pull request.69-69: The use of
integration.TenChainIDin theRandomWalletfunction call aligns with the changes described in the summary and pull request.91-97: The function
createObscuroNetworkstill references "Obscuro" in its name and comments, but it now usesintegration.TenChainID. Consider renaming the function and updating comments to reflect the new context if the Obscuro network is no longer relevant.integration/manualtests/connection_test.go (1)
- 25-25: The change from
NewObscuroGatewayLibrarytoNewTenGatewayLibraryaligns with the project's rebranding efforts and the pull request description. Ensure that theNewTenGatewayLibraryfunction provides the same or improved functionality as the previous library.integration/networktest/actions/setup_actions.go (1)
- 22-27: The change from
integration.ObscuroChainIDtointegration.TenChainIDin the wallet generation process is consistent with the broader rebranding efforts described in the summary. Ensure thatintegration.TenChainIDis correctly defined and that all related functionality works as expected with this new chain ID.integration/networktest/env/testnet.go (1)
- 52-62: The changes to the
testnetConnectorstruct and theChainIDmethod are consistent with the transition from the Obscuro chain to the Ten chain. Ensure that all dependent systems and configurations are updated to accommodate the new chain ID.integration/obscurogateway/gateway_user.go (2)
19-19: The renaming of
ogClienttotgClientin theGatewayUserstruct aligns with the rebranding efforts described in the summary.49-58: The
RegisterAccountsfunction correctly uses the newtgClientvariable name for method calls.integration/simulation/devnetwork/config.go (2)
40-44: The change from
integration.ObscuroChainIDtointegration.TenChainIDis consistent with the renaming process and appears to be correctly implemented in theDefaultDevNetworkfunction.74-77: The change from
integration.ObscuroChainIDtointegration.TenChainIDis consistent with the renaming process and appears to be correctly implemented in theLiveL1DevNetworkfunction.integration/simulation/devnetwork/dev_network.go (2)
61-62: The update of the
ChainIDfunction to returnintegration.TenChainIDaligns with the rebranding efforts described in the summary. Ensure that all references and dependencies throughout the codebase that rely on theChainIDfunction are compatible with this change.59-65: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1-68]
While the provided hunk correctly updates the
ChainID, it is important to ensure that all comments, print statements, and documentation within the file and the rest of the codebase are updated to reflect the rebranding fromObscurotoTen.integration/simulation/devnetwork/node.go (1)
- 125-126: The change from
ObscuroChainIDtoTenChainIDis consistent with the rebranding efforts mentioned in the pull request summary. Ensure that all references to the chain ID throughout the codebase have been updated accordingly.integration/simulation/network/network_utils.go (1)
- 85-86: The change from
integration.ObscuroChainIDtointegration.TenChainIDin thecreateInMemObscuroNodefunction should be carefully reviewed to ensure it aligns with the intended behavior of the system, as it changes the blockchain network context for the node.integration/simulation/simulation_full_network_test.go (1)
- 23-23: The change in chain ID from
integration.ObscuroChainIDtointegration.TenChainIDshould be verified across the entire codebase to ensure consistency and that all references to the old chain ID have been updated.integration/simulation/simulation_geth_in_mem_test.go (1)
- 26-26: The change from
integration.ObscuroChainIDtointegration.TenChainIDin theNewSimWalletsfunction call is consistent with the rebranding efforts described in the pull request summary. Ensure thatTenChainIDis correctly defined and used throughout the entire codebase.integration/simulation/simulation_in_mem_test.go (1)
- 24-24: The update of the chain ID from
integration.ObscuroChainIDtointegration.TenChainIDin thewalletsinitialization is consistent with the broader changes described in the pull request summary. Ensure that this change aligns with the intended blockchain configuration for the simulation.tools/walletextension/common/constants.go (3)
41-41: The addition of the
EncryptedTokenQueryParameterconstant is consistent with the changes described in the summary and pull request.43-43: Verify that the removal of
MessageUserIDLendoes not affect any parts of the codebase that may still rely on this constant.43-43: Verify that the removal of
SignatureLenandPersonalSignMessagePrefixconstants is intentional and does not impact any existing functionality that may depend on them.
| ) | ||
|
|
||
| func TestObscuroscan(t *testing.T) { | ||
| startPort := integration.StartPortObscuroscanUnitTest | ||
| startPort := integration.StartPortTenscanUnitTest | ||
| createObscuroNetwork(t, startPort) | ||
|
|
||
| obsScanConfig := &config.Config{ | ||
| NodeHostAddress: fmt.Sprintf("http://127.0.0.1:%d", startPort+integration.DefaultHostRPCHTTPOffset), | ||
| ServerAddress: fmt.Sprintf("127.0.0.1:%d", startPort+integration.DefaultObscuroscanHTTPPortOffset), | ||
| ServerAddress: fmt.Sprintf("127.0.0.1:%d", startPort+integration.DefaultTenscanHTTPPortOffset), | ||
| LogPath: "sys_out", | ||
| } | ||
| serverAddress := fmt.Sprintf("http://%s", obsScanConfig.ServerAddress) |
There was a problem hiding this comment.
Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [59-59]
The variable obsScanContainer should be renamed to reflect the Tenscan context, such as tenScanContainer.
| @@ -203,7 +203,7 @@ | |||
| // Creates a single-node Obscuro network for testing. | |||
| func createObscuroNetwork(t *testing.T, startPort int) { | |||
There was a problem hiding this comment.
The function createObscuroNetwork should be renamed to reflect the Tenscan context, such as createTenscanNetwork.
| @@ -203,7 +203,7 @@ | |||
| // Creates a single-node Obscuro network for testing. | |||
There was a problem hiding this comment.
The comment should be updated to reflect the new context, replacing "Obscuro" with "Tenscan".
| http() { | ||
| return `${this.httpURL}/v1/?u=${this.userId}`; | ||
| return `${this.httpURL}/v1/?token=${this.userId}`; |
There was a problem hiding this comment.
The http and ws methods should use the obscuroGatewayVersion variable instead of the hardcoded "v1" to maintain consistency and ease of updates.
- return `${this.httpURL}/v1/?token=${this.userId}`;
+ return `${this.httpURL}/${obscuroGatewayVersion}/?token=${this.userId}`;
- return `${this.wsURL}/v1/?token=${this.userId}`;
+ return `${this.wsURL}/${obscuroGatewayVersion}/?token=${this.userId}`;Also applies to: 74-75
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.
| http() { | |
| return `${this.httpURL}/v1/?u=${this.userId}`; | |
| return `${this.httpURL}/v1/?token=${this.userId}`; | |
| http() { | |
| return `${this.httpURL}/${obscuroGatewayVersion}/?token=${this.userId}`; |
| func (o *OGLib) HTTP() string { | ||
| return fmt.Sprintf("%s/v1/?u=%s", o.httpURL, o.userID) | ||
| func (o *TGLib) HTTP() string { | ||
| return fmt.Sprintf("%s/v1/?token=%s", o.httpURL, o.userID) |
There was a problem hiding this comment.
The userID is a byte slice and should be converted to a string before being used in URL construction to avoid potential issues.
- return fmt.Sprintf("%s/v1/?token=%s", o.httpURL, o.userID)
+ return fmt.Sprintf("%s/v1/?token=%s", o.httpURL, string(o.userID))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.
| return fmt.Sprintf("%s/v1/?token=%s", o.httpURL, o.userID) | |
| return fmt.Sprintf("%s/v1/?token=%s", o.httpURL, string(o.userID)) |
|
|
||
| ## User interactions | ||
|
|
||
| The key reason for the OG is to allow implementing a 3-click user onboarding process. | ||
| The key reason for the TG is to allow implementing a 3-click user onboarding process. | ||
|
|
||
| ### User on-boarding |
There was a problem hiding this comment.
The UX diagram filename "og_ux.png" should be updated to match the new project naming convention, such as "tg_ux.png".
| @@ -151,36 +151,36 @@ | |||
| chainId: obscuroChainIDDecimal, | |||
There was a problem hiding this comment.
The chainId in the EIP-712 message format should be updated to reflect the new TenChainID.
| Alice -> OG: Create User\n(ajax call behind the scenes) | ||
| OG -> OG: Generate and record VK\nand record against UserId | ||
| Alice -> TG: Create User\n(ajax call behind the scenes) | ||
| TG -> TG: Generate and record VK\nand record against UserId |
There was a problem hiding this comment.
The term "UserId" should be replaced with "Encryption Token" to maintain consistency with the new identification mechanism.
| @@ -304,14 +305,14 @@ | |||
|
|
|||
| This could be used for tax purposes, or to prove holdings. | |||
|
|
|||
| ## Revocation of userId | |||
| ## Revocation of encryption token | |||
|
|
|||
| Users might suspect someone else knows their UserdId. | |||
There was a problem hiding this comment.
The term "UserId" should be replaced with "Encryption Token" to maintain consistency with the new identification mechanism.
| @@ -42,17 +42,17 @@ | |||
| Wallets: wallets, | |||
| HTTPClient: httpClient, | |||
| WSClient: wsClient, | |||
| ogClient: ogClient, | |||
| tgClient: ogClient, | |||
There was a problem hiding this comment.
The GatewayUser struct initialization should assign the renamed tgClient variable to the tgClient field.
- tgClient: ogClient,
+ tgClient: tgClient,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.
| tgClient: ogClient, | |
| tgClient: tgClient, |
There was a problem hiding this comment.
@coderabbitai pause
There was a problem hiding this comment.
@zkokelj Understood, I'm pausing the actions. Please let me know how you would like to proceed or if there's anything specific you need assistance with.
Why this change is needed
With introduction of
encryptionTokenit doesn't make sense to haveuas query parameter.We decided to add
tokenas an option in query parameters and keepufor now to prevent things from breaking.We will deprecate
uin the future.What changes were made as part of this PR
?u=with?token=PR checks pre-merging
Please indicate below by ticking the checkbox that you have read and performed the required
PR checks