Skip to content

use overloaded web3 instance instead of single provider. required fo… #76

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
srt0422 merged 2 commits into from
Sep 28, 2023
Merged

use overloaded web3 instance instead of single provider. required fo… #76

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d592847
use overloaded web3 instance instead of single provider. required fo…
srt0422 Sep 28, 2023
5f91b54
increment version
srt0422 Sep 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@lumerin/wallet-core",
"version": "1.0.71",
"version": "1.0.72",
"author": {
"name": "Lumerin",
"email": "developer@lumerin.io",
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/contracts/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ function createPlugin() {
const { lmrTokenAddress, cloneFactoryAddress } = config
const { eth } = plugins

const web3 = new Web3(eth.web3Provider)
const web3 = eth.web3
const web3Subscriptionable = new Web3(plugins.eth.web3SubscriptionProvider)
Comment thread
srt0422 marked this conversation as resolved.
Copy link
Copy Markdown

@codereviewbot-ai codereviewbot-ai Bot Sep 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code is creating a new instance of Web3 using plugins.eth.web3SubscriptionProvider. However, there is no error handling in case the creation of the new Web3 instance fails. This could lead to unexpected behavior or crashes if the web3SubscriptionProvider is not properly configured or if there are network issues.

To improve this, you should wrap the creation of the new Web3 instance in a try-catch block and handle any potential errors appropriately. This could involve logging the error, retrying the operation, or failing gracefully with a user-friendly error message.


const lumerin = Lumerin(web3, lmrTokenAddress)
Expand Down
1 change: 1 addition & 0 deletions src/plugins/eth/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ function createPlugin () {

return {
api: {
web3,
web3Provider: web3.currentProvider,
web3SubscriptionProvider: web3SubscribAble.currentProvider,
Comment thread
srt0422 marked this conversation as resolved.
Comment on lines +32 to 34
Copy link
Copy Markdown

@codereviewbot-ai codereviewbot-ai Bot Sep 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code is directly exposing the web3 and web3Provider objects. This could potentially lead to security issues as it allows direct access to the web3 provider and could be misused. Instead of directly exposing these objects, consider providing a set of methods that abstract the operations that can be performed on these objects. This way, you can control what operations are allowed and how they are performed, which can help to prevent misuse and improve security.

},
Expand Down
23 changes: 17 additions & 6 deletions src/plugins/eth/web3.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,8 @@ const overrideFunctions = function (object, providers) {
!key.startsWith('set')
) {
object[key] = function () {
const isAsync =
originalFunctions[key][Symbol.toStringTag] === 'AsyncFunction'
const originalFunction = originalFunctions[key]
const isAsync = originalFunction[Symbol.toStringTag] === 'AsyncFunction'
const args = arguments
Comment thread
srt0422 marked this conversation as resolved.
let providerIndex = lastUsedProviderIndex
let result
Expand All @@ -97,7 +97,7 @@ const overrideFunctions = function (object, providers) {
const provider = providers[providerIndex]
originalSetProvider(provider)
if (isAsync) {
result = originalFunctions[key]
result = originalFunction
.apply(this, args)
.then((res) => {
if (res !== undefined) {
Expand All @@ -111,8 +111,19 @@ const overrideFunctions = function (object, providers) {
})
} else {
try {
result = originalFunctions[key].apply(this, args)
if (result !== undefined) {
if (new.target) {
function F(args) {
return originalFunction.apply(this, args)
}

F.prototype = originalFunction.prototype

return new F(args)
} else {
result = originalFunctions[key].apply(this, args)
}

if (typeof result !== "undefined") {
break
}
Comment on lines 111 to 128
Copy link
Copy Markdown

@codereviewbot-ai codereviewbot-ai Bot Sep 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error handling in this code is not sufficient. In the catch block, the error is caught but nothing is done with it. This can make debugging difficult because you lose information about what caused the error.

To improve this, you should at least log the error. Depending on your application's requirements, you might also want to handle specific types of errors differently. For example, you might want to retry on certain types of errors, or report them to an error tracking service.

} catch (error) {
Expand All @@ -121,7 +132,7 @@ const overrideFunctions = function (object, providers) {
}
} while (providerIndex !== lastUsedProviderIndex)
lastUsedProviderIndex = providerIndex
if (result === undefined) {
if (typeof result === "undefined") {
throw new Error('All providers failed to execute the function')
}
return result
Expand Down