Add Obscuro Gateway Widget Package in Tools #1620
Conversation
WalkthroughThe pull request introduces a new package, "obscuro-widget", which provides a widget for integrating with the Obscuro Gateway. It includes the widget's installation and usage instructions, and the complete implementation of the widget's functionality, such as joining the Obscuro network, account authentication, and Ethereum-related operations. 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: 7
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- tools/gatewaywidget/package.json
- tools/gatewaywidget/src/menu.svg
Files selected for processing (2)
- tools/gatewaywidget/README.md (1 hunks)
- tools/gatewaywidget/src/index.js (1 hunks)
Files skipped from review due to trivial changes (1)
- tools/gatewaywidget/README.md
Additional comments: 1
tools/gatewaywidget/src/index.js (1)
- 394-398: The function
getRandomIntAsStringis used to generate a random integer as a string. However, it's not clear why this function is needed. If it's used for generating unique IDs, consider using a more robust method like UUIDs.
| border: 1px solid #ff6262; | ||
| } | ||
|
|
||
| #options-menu { | ||
| position: absolute; | ||
| top: 80px; /* Adjust this as needed */ | ||
| right: 15px; | ||
| background-color: #1D1D1D; | ||
| color: white; | ||
| box-shadow: 0px 0px 5px 0.5px rgba(0,0,0,0.75); | ||
| z-index: 10000; | ||
| width: 150px; /* Or whatever width you desire */ | ||
| cursor: pointer; | ||
| } | ||
|
|
||
| #options-menu ul { | ||
| list-style: none; | ||
| padding: 0; | ||
| margin: 0; | ||
| } | ||
|
|
||
| #options-menu li { | ||
| padding: 10px; | ||
| border-bottom: 1px solid #353535; | ||
| } | ||
|
|
||
| #options-menu li:last-child { | ||
| border-bottom: none; | ||
| } | ||
|
|
||
| h3 { | ||
| color: #fff; | ||
| } | ||
|
|
||
| hr { | ||
| border: 0; | ||
| border-top: 1px solid #353535; /* Dark line */ | ||
| margin-bottom: 15px; | ||
| } | ||
|
|
||
| button { | ||
| margin: 5px; | ||
| padding: 10px 15px; | ||
| background-color: #00a8e6; | ||
| color: #fff; | ||
| border: 1px solid #00a8e6; | ||
| border-radius: 7.5px; | ||
| transition: background-color 0.3s ease; | ||
| width: 135px; | ||
| display: block; /* Makes the button behave like a block-level element */ | ||
| margin-left: auto; /* Centers the button */ | ||
| margin-right: auto; | ||
| cursor: pointer; | ||
| } | ||
|
|
||
| button:hover { | ||
| background-color: #555; | ||
| border: 1px solid #00a8e6; | ||
| } | ||
|
|
||
| #accountsTable { | ||
| width: 100%; | ||
| margin-top: 20px; | ||
| border-collapse: collapse; | ||
| border-spacing: 0; | ||
| border-radius: 10px; | ||
| overflow: hidden; | ||
| } | ||
|
|
||
| #accountsTable th, #accountsTable td { | ||
| padding: 10px; | ||
| color: #fff; | ||
| background-color: #222222; | ||
| text-align: center; | ||
| border: 0.01em solid #353535; | ||
| } | ||
|
|
||
| #accountsTable th { | ||
| background-color: #000000; | ||
| color: white; | ||
| } | ||
|
|
||
| #status { | ||
| margin-top: 10px; | ||
| color: #ffae00; | ||
| font-size: 15px; | ||
| text-align: center; | ||
| } | ||
|
|
||
| .hidden { | ||
| display: none; | ||
| } | ||
|
|
||
| .close-logo { | ||
| font-size: 30px; | ||
| display: flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
| } | ||
| `; |
There was a problem hiding this comment.
The CSS styles are embedded directly into the JavaScript code. This can make the code harder to maintain and debug. Consider separating the CSS into a separate file and linking it in the HTML where the widget is used. This will also allow for better separation of concerns.
| (function() { | ||
| const widgetHTML = `<!-- Obscuro Gateway Widget HTML Code --> | ||
| <button id="obscuro-button"> | ||
| <span class="open-logo">◠.</span> | ||
| <span class="close-logo hidden">X</span> | ||
| </button> | ||
|
|
||
| <!-- Obscuro Gateway Panel --> | ||
| <div id="obscuro-panel" class="hidden"> | ||
| <div> | ||
| <!-- Options Menu Button --> | ||
| <div id="options">⫶</div> | ||
|
|
||
| <!-- Dropdown Menu Content --> | ||
| <div id="options-menu" class="hidden"> | ||
| <ul> | ||
| <li><div id="revokeUserID">Revoke UserID</div></li> | ||
| </ul> | ||
| </div> | ||
| <h3>Obscuro Gateway</h3> | ||
| <hr> | ||
|
|
||
| <!-- Join Obscuro Network --> | ||
| <button id="join">JOIN OBSCURO</button> | ||
|
|
||
| <!-- Status Area --> | ||
| <div id="status"></div> | ||
|
|
||
| <!-- Accounts Table --> | ||
| <table id="accountsTable"> | ||
| <thead> | ||
| <tr> | ||
| <th>Account</th> | ||
| <th>Status</th> | ||
| </tr> | ||
| </thead> | ||
| <tbody id="tableBody"></tbody> | ||
| </table> | ||
| </div> | ||
| </div> | ||
|
|
||
| <!-- Add necessary styles for the hidden class --> | ||
| <style> | ||
| .hidden { | ||
| display: none; | ||
| } | ||
| </style>`; | ||
|
|
||
| // Create a new div for the widget and set its innerHTML | ||
| const widgetContainer = document.createElement('div'); | ||
| widgetContainer.innerHTML = widgetHTML; | ||
|
|
||
| // Append the widget container to the document body | ||
| document.body.appendChild(widgetContainer); | ||
| })(); |
There was a problem hiding this comment.
The HTML for the widget is also embedded directly into the JavaScript code. Similar to the CSS, this can make the code harder to maintain and debug. Consider separating the HTML into a separate file and loading it dynamically or including it in the HTML where the widget is used.
| function isValidUserIDFormat(value) { | ||
| return typeof value === 'string' && value.length === 64; | ||
| } |
There was a problem hiding this comment.
This function checks if a value is a string and if its length is 64. However, it doesn't check if the string contains only valid characters (e.g., hexadecimal characters). Consider adding a regular expression check to ensure the string contains only valid characters.
| // add network to MetaMask | ||
| try { | ||
| await ethereum.request({ | ||
| method: 'wallet_addEthereumChain', | ||
| params: [ | ||
| { | ||
| chainId: obscuroChainIDHex, | ||
| chainName: getNetworkName(obscuroGatewayAddress), | ||
| nativeCurrency: { | ||
| name: 'Sepolia Ether', | ||
| symbol: 'ETH', | ||
| decimals: 18 | ||
| }, | ||
| rpcUrls: [getRPCFromUrl(obscuroGatewayAddress)+"/"+obscuroGatewayVersion+'/?u='+userID], | ||
| blockExplorerUrls: ['https://testnet.obscuroscan.io'], | ||
| }, | ||
| ], | ||
| }); | ||
| } catch (error) { | ||
| console.error(error); | ||
| return false | ||
| } |
There was a problem hiding this comment.
The function addNetworkToMetaMask doesn't handle the case where the user rejects the request to add the Ethereum chain. Consider adding error handling for this case to provide feedback to the user.
| function checkIfMetamaskIsLoaded() { | ||
| if (window.ethereum) { | ||
| handleEthereum(); | ||
| } else { | ||
| const statusArea = document.getElementById(idStatus); | ||
| const table = document.getElementById("accountsTable"); | ||
| table.style.display = "none"; | ||
| statusArea.innerText = 'Connecting to Metamask...'; | ||
| window.addEventListener('ethereum#initialized', handleEthereum, { | ||
| once: true, | ||
| }); | ||
|
|
||
| // If the event is not dispatched by the end of the timeout, | ||
| // the user probably doesn't have MetaMask installed. | ||
| setTimeout(handleEthereum, 3000); // 3 seconds | ||
| } | ||
| } | ||
|
|
||
| function handleEthereum() { | ||
| const { ethereum } = window; | ||
| if (ethereum && ethereum.isMetaMask) { | ||
| provider = new ethers.providers.Web3Provider(window.ethereum); | ||
| initialize() | ||
| } else { | ||
| const statusArea = document.getElementById(idStatus); | ||
| statusArea.innerText = 'Please install MetaMask to use Obscuro Gateway.'; | ||
| } | ||
| } |
There was a problem hiding this comment.
The function checkIfMetamaskIsLoaded checks if MetaMask is loaded and if not, it sets a timeout to check again after 3 seconds. However, it doesn't handle the case where MetaMask is still not loaded after 3 seconds. Consider adding a loop or recursive function to keep checking until MetaMask is loaded or a maximum number of attempts has been reached.
| async function isObscuroChain() { | ||
| let currentChain = await ethereum.request({ method: 'eth_chainId' }); | ||
| return currentChain === obscuroChainIDHex | ||
| } | ||
|
|
||
| async function switchToObscuroNetwork() { | ||
| try { | ||
| await ethereum.request({ | ||
| method: 'wallet_switchEthereumChain', | ||
| params: [{ chainId: obscuroChainIDHex }], | ||
| }); | ||
| return 0 | ||
| } catch (switchError) { | ||
| return switchError.code | ||
| } | ||
| return -1 | ||
| } |
There was a problem hiding this comment.
The function switchToObscuroNetwork returns different types of values: a number (0), an error code (which is also a number), or -1. This can make the function harder to use because the caller needs to handle different types of return values. Consider standardizing the return value to always be a boolean or always be an error code.
| optionsMenu.style.display = "none"; | ||
| accountsTable.style.display = "block" | ||
| await populateAccountsTable(document, tableBody, userID) | ||
| statusArea.innerText = "Successfully connected"; | ||
| } | ||
|
|
||
| async function displayCorrectScreenBasedOnMetamaskAndUserID() { | ||
| // check if we are on Obscuro Chain | ||
| if(await isObscuroChain()){ | ||
| // check if we have valid userID in rpcURL | ||
| if (isValidUserIDFormat(userID)) { | ||
| return await displayConnectedAndJoinedSuccessfully() | ||
| } | ||
| } | ||
| return displayOnlyJoin() | ||
| } | ||
|
|
||
| // load the current version | ||
| await fetchAndDisplayVersion(); | ||
|
|
||
| await displayCorrectScreenBasedOnMetamaskAndUserID() | ||
|
|
||
| joinButton.addEventListener(eventClick, async () => { | ||
| // check if we are on an obscuro chain | ||
| if (await isObscuroChain()) { | ||
| userID = await getUserID() | ||
| if (!isValidUserIDFormat(userID)) { | ||
| statusArea.innerText = "Please remove existing Obscuro network from metamask and start again." | ||
| } | ||
| } else { | ||
| // we are not on an Obscuro network - try to switch | ||
| let switched = await switchToObscuroNetwork(); | ||
| // error 4902 means that the chain does not exist | ||
| if (switched === 4902 || !isValidUserIDFormat(await getUserID())) { | ||
| // join the network | ||
| const joinResp = await fetch( | ||
| pathJoin, { | ||
| method: methodGet, | ||
| headers: jsonHeaders, | ||
| }); | ||
| if (!joinResp.ok) { | ||
| console.log("Error joining Obscuro Gateway") | ||
| statusArea.innerText = "Error joining Obscuro Gateway. Please try again later." | ||
| return | ||
| } | ||
| userID = await joinResp.text(); | ||
|
|
||
| // add Obscuro network | ||
| await addNetworkToMetaMask(window.ethereum, userID) | ||
| } | ||
|
|
||
| // we have to check if user has accounts connected with metamask - and promt to connect if not | ||
| if (!await isMetamaskConnected()) { | ||
| await connectAccounts(); | ||
| } | ||
|
|
||
| // connect all accounts | ||
| // Get an accounts and prompt user to sign joining with a selected account | ||
| const accounts = await provider.listAccounts(); | ||
| if (accounts.length === 0) { | ||
| statusArea.innerText = "No MetaMask accounts found." | ||
| return | ||
| } | ||
|
|
||
| userID = await getUserID(); | ||
| for (const account of accounts) { | ||
| await authenticateAccountWithObscuroGateway(ethereum, account, userID) | ||
| accountsTable.style.display = "block" | ||
| await populateAccountsTable(document, tableBody, userID) | ||
| } | ||
|
|
||
| // if accounts change we want to give user chance to add them to Obscuro | ||
| window.ethereum.on('accountsChanged', async function (accounts) { | ||
| if (isValidUserIDFormat(await getUserID())) { | ||
| userID = await getUserID(); | ||
| for (const account of accounts) { | ||
| await authenticateAccountWithObscuroGateway(ethereum, account, userID) | ||
| accountsTable.style.display = "block" | ||
| await populateAccountsTable(document, tableBody, userID) | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| await displayConnectedAndJoinedSuccessfully() | ||
| } | ||
| }) | ||
|
|
||
| revokeUserIDButton.addEventListener(eventClick, async () => { | ||
| let result = await revokeUserID(userID); | ||
|
|
||
| await populateAccountsTable(document, tableBody, userID) | ||
|
|
||
| if (result) { | ||
| displayOnlyJoin() | ||
| statusArea.innerText = "Revoked UserID"; | ||
| } else { | ||
| statusArea.innerText = "Revoking UserID failed"; | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
The initialize function is quite long and does a lot of different things. Consider breaking it up into smaller functions to improve readability and maintainability. For example, you could have separate functions for handling the join button click, the revoke button click, and the accounts changed event.
There was a problem hiding this comment.
There's a few things that should change here:
- Package building ( lint, test, build dist )
- Npm package format ( currently it's an embadable js file )
- The readme isn't correct (
npm install obscuro-widgetdoesnt work) - Deployment into gh pages
- The format of the index.js is weird as it's wrapping everything in js, seem to be a dist file
Perhaps there is no need to make big changes in the index.js code at the moment, but it would be important to have the same format for these base rails as the gateway-lib, to have these base points, which the gateway-js can be used as reference.
Can these be added before merging in ?
- build and deploy yml files
- index.js be a npm base package that is compiled into a dist
|
@otherview , @0xjba , what's the status of this pr? |
Why this change is needed
Please provide a description and a link to the underlying ticket
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