Initial commit for basic gateway package #1619
Conversation
WalkthroughThis pull request introduces a new JavaScript library, "Obscuro Gateway JS", for interacting with the Obscuro network. It includes the addition of a 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: 8
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- tools/gateway-js/gateway-lib/package-lock.json
- tools/gateway-js/gateway-lib/package.json
Files selected for processing (7)
- .github/workflows/build-gateway-lib.yml (1 hunks)
- .github/workflows/deploy-gateway-lib.yml (1 hunks)
- tools/gateway-js/gateway-lib/.eslintrc.js (1 hunks)
- tools/gateway-js/gateway-lib/dist/gateway.bundle.js (1 hunks)
- tools/gateway-js/gateway-lib/gateway.js (1 hunks)
- tools/gateway-js/gateway-lib/gateway.test.js (1 hunks)
- tools/gateway-js/gateway-lib/webpack.config.js (1 hunks)
Files skipped from review due to trivial changes (4)
- .github/workflows/build-gateway-lib.yml
- .github/workflows/deploy-gateway-lib.yml
- tools/gateway-js/gateway-lib/.eslintrc.js
- tools/gateway-js/gateway-lib/dist/gateway.bundle.js (diff too large)
Additional comments (Suppressed): 4
tools/gateway-js/gateway-lib/gateway.js (4)
1-3: The
axioslibrary is used for HTTP requests. This is a good choice as it provides a wide range of features and is widely used in the JavaScript community. However, it's important to ensure that the version ofaxiosbeing used is up-to-date and free of known vulnerabilities.40-42: The method checks if the signature is -1 and returns "Signing failed" if it is. This seems to be a specific error condition, but it's not clear why -1 would indicate a failure. It would be helpful to add a comment explaining this.
63-73: The
userID,http, andwsmethods are simple getter methods that return the user ID, HTTP URL, and WebSocket URL, respectively. These are straightforward and don't seem to have any issues.76-76: The
Gatewayclass is exported as a module. This allows it to be imported and used in other parts of the codebase. This is a good practice for modularity and code reuse.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- tools/gateway-js/gateway-lib/gateway.test.js (1 hunks)
Additional comments (Suppressed): 6
tools/gateway-js/gateway-lib/gateway.test.js (6)
16-18: The
beforeEachblock is still being used to initialize a newGatewayinstance before each test. However, in the tests forregisterAccount(lines 42 and 52), a newGatewayinstance is created again with a mockedprovider. This redundancy has not been addressed yet. Consider moving theprovidermock to thebeforeEachblock and remove the additionalGatewayinstance creation in theregisterAccounttests.38-40: The
fetchmock is still set to resolve with an object that has atextmethod, which is also a mock that resolves with 'Account registered'. This simulates the behavior of thefetchAPI, which returns a Promise that resolves with a Response object, and thetextmethod of the Response object also returns a Promise. However, it would be more accurate to simulate a real Response object by including astatusproperty in the mock. This would also allow you to write tests for different HTTP status codes.20-29: The test 'should join successfully' mocks the
axios.getmethod to resolve with a status of 200 and a data of 'testUserID'. It then calls thejoinmethod of theGatewayinstance and checks if theuserIDproperty of theGatewayinstance is set to 'testUserID'. This is a good test for the successful execution of thejoinmethod.31-35: The test 'should throw error on unsuccessful join' mocks the
axios.getmethod to reject with a 'Network error'. It then calls thejoinmethod of theGatewayinstance and checks if it throws the expected error. This is a good test for the error handling in thejoinmethod.37-49: The test 'should register account successfully' mocks the
fetchmethod and therequestmethod of theproviderto resolve with 'Account registered' and 'mockSignature' respectively. It then calls theregisterAccountmethod of theGatewayinstance and checks if it returns 'Account registered'. This is a good test for the successful execution of theregisterAccountmethod.51-57: The test 'should throw error on unsuccessful account registration' mocks the
requestmethod of theproviderto reject with a 'Signature error'. It then calls theregisterAccountmethod of theGatewayinstance and checks if it throws the expected error. This is a good test for the error handling in theregisterAccountmethod.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files selected for processing (4)
- tools/gateway-js/gateway-lib/README.md (1 hunks)
- tools/gateway-js/gateway-lib/dist/gateway.bundle.js (1 hunks)
- tools/gateway-js/gateway-lib/gateway.js (1 hunks)
- tools/gateway-js/gateway-lib/gateway.test.js (1 hunks)
Files skipped from review due to trivial changes (2)
- tools/gateway-js/gateway-lib/README.md
- tools/gateway-js/gateway-lib/dist/gateway.bundle.js (diff too large)
Additional comments (Suppressed): 4
tools/gateway-js/gateway-lib/gateway.test.js (1)
- 16-18: The previous comment about redundancy in creating new
Gatewayinstances in theregisterAccounttests still stands. Consider refactoring thebeforeEachblock to include a mockedproviderand remove the additionalGatewayinstance creation in theregisterAccounttests.- beforeEach(() => { - gateway = new Gateway(httpURL, wsURL, provider); - }); + beforeEach(() => { + provider = { + request: jest.fn().mockResolvedValue("mockSignature") + }; + gateway = new Gateway(httpURL, wsURL, provider); + });tools/gateway-js/gateway-lib/gateway.js (3)
1-3: The
axioslibrary is being used for HTTP requests. This is a popular choice due to its promise-based nature and ease of use. However, it's important to ensure that the version ofaxiosbeing used is up-to-date and free of known vulnerabilities. Please verify this.62-72: The
userID,http, andwsmethods are used to retrieve the user ID and the HTTP and WebSocket URLs. These are simple getter methods and seem to be implemented correctly.75-75: The
Gatewayclass is being exported as a module. This allows it to be imported and used in other parts of the application. This is a standard practice in Node.js and seems to be implemented correctly.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (3)
- tools/gateway-js/gateway-lib/dist/gateway.bundle.js (1 hunks)
- tools/gateway-js/gateway-lib/gateway.js (1 hunks)
- tools/gateway-js/gateway-lib/gateway.test.js (1 hunks)
Files skipped from review due to trivial changes (2)
- tools/gateway-js/gateway-lib/dist/gateway.bundle.js
- tools/gateway-js/gateway-lib/gateway.test.js
Additional comments: 1
tools/gateway-js/gateway-lib/gateway.js (1)
- 42-44: The check for
signature === -1is still unclear. It would be helpful to add a comment explaining why this specific value indicates a failure, as it's not immediately clear from the context.
| async registerAccount(address) { | ||
| const message = `Register ${this.userID} for ${address.toLowerCase()}`; | ||
| let signature = "" | ||
|
|
||
| try { | ||
| signature = await this.provider.request({ | ||
| method: "personal_sign", | ||
| params: [message, address] | ||
| }) | ||
| } catch (err) { | ||
| throw new Error(`Failed to sign message. ${err}`); | ||
| } | ||
|
|
||
| // todo make further checks on the data | ||
| if (signature === -1) { | ||
| return "Signing failed" | ||
| } | ||
|
|
||
| try { | ||
| const authenticateUserURL = pathAuthenticate+"?u="+this.userId | ||
| const authenticateFields = {"signature": signature, "message": message} | ||
| const authenticateResp = await axios.post( | ||
| authenticateUserURL, | ||
| authenticateFields, | ||
| { | ||
| headers: { | ||
| "Accept": "application/json", | ||
| "Content-Type": "application/json" | ||
| } | ||
| } | ||
| ); | ||
| // todo make further checks on the data | ||
| return await authenticateResp.text() | ||
| } catch (error) { | ||
| throw new Error(`Failed to register account. ${error}`); | ||
| } |
There was a problem hiding this comment.
The registerAccount method constructs a message and signs it using the provider, but it does not handle the case where the provider fails to sign the message. This could lead to unexpected behavior or errors later on. Consider adding a check to ensure that signature is not null or empty before proceeding with the authentication request.
| userID() { | ||
| return this.userId; | ||
| } | ||
|
|
||
| http() { | ||
| return `${this.httpURL}/v1/?u=${this.userId}`; | ||
| } | ||
|
|
||
| ws() { | ||
| return `${this.wsURL}/v1/?u=${this.userId}`; | ||
| } |
There was a problem hiding this comment.
The userID, http, and ws methods return values derived from this.userId, but do not handle the case where this.userId is null or empty. This could lead to unexpected behavior or errors later on. Consider adding checks to ensure that this.userId is not null or empty before returning these values.
const axios = require("axios");
class Gateway {
constructor(httpURL, wsURL, provider, obscuroGatewayVersion = "v1") {
this.httpURL = httpURL;
this.wsURL = wsURL;
this.userId = '';
this.provider = provider;
this.pathJoin = obscuroGatewayVersion + "/join/";
this.pathAuthenticate = obscuroGatewayVersion + "/authenticate/";
}
async join() {
try {
const response = await axios.get(`${this.httpURL}${this.pathJoin}`);
if (response.status !== 200 || !response.data) {
throw new Error(`Failed to get userID. Status code: ${response.status}`);
}
// todo make further checks on the data
this.userId = response.data;
} catch (error) {
throw new Error(`Failed to get userID. ${error}`);
}
}
async registerAccount(address) {
const message = `Register ${this.userID} for ${address.toLowerCase()}`;
let signature = ""
try {
signature = await this.provider.request({
method: "personal_sign",
params: [message, address]
})
} catch (err) {
throw new Error(`Failed to sign message. ${err}`);
}
// todo make further checks on the data
if (signature === -1 || !signature) {
return "Signing failed"
}
try {
const authenticateUserURL = this.pathAuthenticate+"?u="+this.userId
const authenticateFields = {"signature": signature, "message": message}
const authenticateResp = await axios.post(
authenticateUserURL,
authenticateFields,
{
headers: {
"Accept": "application/json",
"Content-Type": "application/json"
}
}
);
// todo make further checks on the data
return await authenticateResp.text()
} catch (error) {
throw new Error(`Failed to register account. ${error}`);
}
}
userID() {
if (!this.userId) {
throw new Error("User ID is not set.");
}
return this.userId;
}
http() {
if (!this.userId) {
throw new Error("User ID is not set.");
}
return `${this.httpURL}/v1/?u=${this.userId}`;
}
ws() {
if (!this.userId) {
throw new Error("User ID is not set.");
}
return `${this.wsURL}/v1/?u=${this.userId}`;
}
}
module.exports = Gateway;Committable suggestion (Beta)
| userID() { | |
| return this.userId; | |
| } | |
| http() { | |
| return `${this.httpURL}/v1/?u=${this.userId}`; | |
| } | |
| ws() { | |
| return `${this.wsURL}/v1/?u=${this.userId}`; | |
| } | |
| const axios = require("axios"); | |
| class Gateway { | |
| constructor | |
| <!-- This is an auto-generated comment by CodeRabbit --> |
Why this change is needed
https://github.com/obscuronet/obscuro-internal/issues/2321
This PR adds a gateway.js package that allows for the centralization of js operations that were being copied and duplicated in a few upcoming tools. Also adds tooling so that the js can be published and other tools can include it.
Tools can now include it as an NPM package or directly import it.
It's still missing the update to the wallet extension FE, but that will only work after the js is deployed as WE is a pure FE without Nodejs packaging.