Skip to content

Validator registry integration #84

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
shev-titan merged 2 commits into from
Feb 2, 2025
Merged

Validator registry integration #84

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
b2e85eb
Added new plugin
bohdan-titan Jan 24, 2025
ec92156
feat: validator registry contract integration
lsheva Feb 2, 2025
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
8 changes: 4 additions & 4 deletions 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.1.2",
"version": "1.1.4",
"author": {
"name": "Lumerin",
"email": "developer@lumerin.io",
Expand Down Expand Up @@ -36,8 +36,7 @@
"axios-cookiejar-support": "1.0.1",
"bottleneck": "^2.19.5",
"chalk": "^2.4.2",
"cheerio": "^1.0.0-rc.12",
"contracts-js": "github:Lumerin-protocol/contracts-js#v0.1.0",
"contracts-js": "github:Lumerin-protocol/contracts-js#v1.2.1",
"cross-port-killer": "^1.4.0",
"debug": "4.1.1",
"ecies-geth": "^1.7.0",
Expand All @@ -58,6 +57,7 @@
"websocket-reconnector": "1.1.1"
},
"devDependencies": {
"@dopex-io/web3-multicall": "^0.1.10",
"chai": "4.3.4",
"chai-as-promised": "7.1.1",
"check-tag-matches": "1.0.0",
Expand All @@ -84,4 +84,4 @@
"engines": {
"node": ">=12"
}
}
}
1 change: 1 addition & 0 deletions src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ const pluginCreators = [
require('./plugins/proxy-router'),
require('./plugins/contracts'),
require('./plugins/devices'),
require('./plugins/validator-registry')
]
Comment on lines 14 to 18
Copy link
Copy Markdown

@codereviewbot-ai codereviewbot-ai Bot Jan 24, 2025

Choose a reason for hiding this comment

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

Performance and Maintainability Issue

The synchronous loading of plugins using require within the pluginCreators array can lead to performance bottlenecks, especially if the initialization of these plugins is resource-intensive. This approach also makes the system less flexible and harder to maintain because any modifications to the plugins require changes to this core file.

Recommendation: Consider using an asynchronous plugin loading mechanism, such as dynamic imports (import()) which can be used to load modules on demand. This would not only improve the load times but also enhance the system's ability to scale and adapt to changes more gracefully.

Comment on lines 14 to 18
Copy link
Copy Markdown

@codereviewbot-ai codereviewbot-ai Bot Jan 24, 2025

Choose a reason for hiding this comment

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

Modularity and Scalability Issue

Hardcoding the paths of plugin modules in the pluginCreators array reduces the system's modularity and scalability. This approach requires manual updates to the core system file whenever plugins are added or removed, increasing the risk of errors and complicating the maintenance process.

Recommendation: Implement a more dynamic plugin registration system, possibly through a configuration file or a directory scanning mechanism that automatically registers all plugins. This would enhance modularity and make the system more robust and easier to scale as the number of plugins grows.


function createCore() {
Expand Down
68 changes: 37 additions & 31 deletions src/plugins/contracts/api.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,11 @@ const logger = require('../../logger')
const { encrypt } = require('ecies-geth')
const { Implementation } = require('contracts-js')
const { remove0xPrefix, add65BytesPrefix } = require('./helpers')
const { decompressPublicKey } = require('../validator-registry/api')
const ethereumWallet = require('ethereumjs-wallet').default

const ZERO_ADDRESS = "0x0000000000000000000000000000000000000000"

/**
* @param {import('web3').default} web3
* @param {string} implementationAddress
Expand Down Expand Up @@ -45,7 +48,7 @@ async function _loadContractInstance(
_length: length, // duration of the contract in seconds
_version: version,
_profitTarget: profitTarget
},
},
_startingBlockTimestamp: timestamp, // timestamp of the block at moment of purchase
_buyer: buyer, // wallet address of the purchasing party
_seller: seller, // wallet address of the selling party
Expand Down Expand Up @@ -253,73 +256,76 @@ function setContractDeleteStatus(web3, cloneFactory, onUpdate) {
}
}


/**
*
* @param {import('web3').default} web3
* @param {import('contracts-js').CloneFactoryContext} cloneFactory
* @param {import('contracts-js').LumerinContext} lumerin
* @returns
* @returns {(p: {validatorUrl: string, validatorAddr: string, destUrl: string, validatorPubKeyYparity: boolean, validatorPubKeyX: `0x${string}`, contractAddr: string, price: string,version: number, privateKey: string}) => Promise<void>}
*/
function purchaseContract(web3, cloneFactory, lumerin) {
return async (params) => {
const { walletId, contractId, url, privateKey, price, version } = params
const sendOptions = { from: walletId }
const { validatorUrl, destUrl, validatorAddr, validatorPubKeyYparity, validatorPubKeyX, privateKey, contractAddr, price, version } = params;

//getting pubkey from contract to be purchased
const implementationContract = Implementation(web3, contractId)

const pubKey = await implementationContract.methods.pubKey().call()

//encrypting plaintext url parameter
const ciphertext = await encrypt(
Buffer.from(add65BytesPrefix(pubKey), 'hex'),
Buffer.from(url)
)

const account = web3.eth.accounts.privateKeyToAccount(privateKey)
web3.eth.accounts.wallet.create(0).add(account)

const {
data: { isDead, price: p },
} = await _loadContractInstance(web3, contractId)
if (isDead) {
throw new Error('Contract is deleted already')
}
const implementationContract = Implementation(web3, contractAddr)
const sellerPubKey = await implementationContract.methods.pubKey().call()

const isThirdPartyValidator = validatorAddr !== ZERO_ADDRESS;
const encrDestPubKey = isThirdPartyValidator ? decompressPublicKey(validatorPubKeyYparity, validatorPubKeyX) : sellerPubKey;

const encrValidatorUrl = await encrypt(
Buffer.from(add65BytesPrefix(sellerPubKey), 'hex'),
Buffer.from(validatorUrl)
).then(res => res.toString('hex'))

const encrDestUrl = await encrypt(
Buffer.from(add65BytesPrefix(encrDestPubKey), 'hex'),
Buffer.from(destUrl)
).then(res => res.toString('hex'))

const increaseAllowanceEstimate = await lumerin.methods
.increaseAllowance(cloneFactory.options.address, price)
.estimateGas({
from: walletId,
from: account.address,
})

await lumerin.methods
.increaseAllowance(cloneFactory.options.address, price)
.send({
from: walletId,
from: account.address,
gas: increaseAllowanceEstimate,
})

const marketplaceFee = await cloneFactory.methods.marketplaceFee().call()

const purchaseGas = await cloneFactory.methods
.setPurchaseRentalContract(
contractId,
ciphertext.toString('hex'),
.setPurchaseRentalContractV2(
contractAddr,
validatorAddr,
encrValidatorUrl,
encrDestUrl,
version
)
.estimateGas({
from: sendOptions.from,
from: account.address,
value: marketplaceFee,
})

const purchaseResult = await cloneFactory.methods
.setPurchaseRentalContract(
contractId,
ciphertext.toString('hex'),
.setPurchaseRentalContractV2(
contractAddr,
validatorAddr,
encrValidatorUrl,
encrDestUrl,
version
)
.send({
...sendOptions,
from: account.address,
gas: purchaseGas,
value: marketplaceFee,
})
Comment on lines 256 to 331
Copy link
Copy Markdown

@codereviewbot-ai codereviewbot-ai Bot Feb 2, 2025

Choose a reason for hiding this comment

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

Security Concern: Insufficient Input Validation

The function purchaseContract does not perform any validation on the inputs it receives, such as validatorUrl, destUrl, validatorAddr, validatorPubKeyYparity, validatorPubKeyX, privateKey, contractAddr, price, and version. This lack of validation could lead to various issues, including sending incorrect data to the blockchain, which can be costly and irreversible.

Recommended Solution:
Implement input validation checks at the beginning of the purchaseContract function. Ensure that all inputs are of the expected type and format before proceeding with further processing. This will help prevent potential errors or malicious inputs from affecting the contract interaction logic.

Expand Down Expand Up @@ -357,7 +363,7 @@ function editContract(web3, cloneFactory, lumerin) {
.estimateGas({
from: sendOptions.from,
});

const editResult = await cloneFactory.methods
.setUpdateContractInformationV2(contractId, price, limit, speed, duration, +profit)
.send({
Comment on lines 363 to 369
Copy link
Copy Markdown

@codereviewbot-ai codereviewbot-ai Bot Feb 2, 2025

Choose a reason for hiding this comment

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

Performance Optimization: Redundant Transaction Estimation

In the editContract function, the gas estimation for the transaction is performed twice (lines 363 and 367) with the same parameters. This redundancy not only wastes computational resources but also increases the time the function takes to execute, which could lead to a poorer user experience.

Recommended Solution:
Remove the redundant gas estimation call. Perform the gas estimation once and use the result for the transaction submission. This change will optimize the function's performance by reducing unnecessary blockchain calls and speeding up the transaction process.

Expand Down
12 changes: 11 additions & 1 deletion src/plugins/contracts/helpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,17 @@
const remove0xPrefix = privateKey => privateKey.replace('0x', '');

// https://superuser.com/a/1465498
const add65BytesPrefix = key => `04${key}`;
/** @param {string} key */
const add65BytesPrefix = key => {
key = key.replace("0x", "")
Copy link
Copy Markdown

@codereviewbot-ai codereviewbot-ai Bot Feb 2, 2025

Choose a reason for hiding this comment

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

Redundancy in Code

The remove0xPrefix functionality is duplicated in add65BytesPrefix. This redundancy can be eliminated by using remove0xPrefix within add65BytesPrefix, which would improve code maintainability and consistency.

Recommendation:
Refactor add65BytesPrefix to use remove0xPrefix for removing the '0x' prefix. This will ensure that any modifications to the prefix removal logic are centralized, making the code easier to maintain and less error-prone.


// 64 bytes hex string (2 char per byte)
if (key.length === 64 * 2) {
return `04${key}`
}

return key
}
Comment on lines 3 to +16
Copy link
Copy Markdown

@codereviewbot-ai codereviewbot-ai Bot Feb 2, 2025

Choose a reason for hiding this comment

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

Lack of Input Validation and Error Handling

Both remove0xPrefix and add65BytesPrefix functions do not perform any input validation or error handling. This can lead to issues if non-string inputs are passed or if the strings do not conform to expected formats, potentially causing runtime errors or incorrect processing.

Recommendation:
Implement input validation to ensure that the functions receive properly formatted strings. For instance, check if the input is a string and if it contains the expected characters before processing. This will make the functions more robust and prevent errors during runtime.


module.exports = {
remove0xPrefix,
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/devices/configuration-strategies/ant-miner-strategy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
const AxiosDigestAuth = require('@mhoc/axios-digest-auth')
const cheerio = require('cheerio')
// const cheerio = require('cheerio')
Copy link
Copy Markdown

@codereviewbot-ai codereviewbot-ai Bot Feb 2, 2025

Choose a reason for hiding this comment

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

The commented-out import of cheerio on line 2 might be confusing. If cheerio is not needed, it's better to remove this line to avoid confusion and keep the code clean. If it might be needed in the future, consider adding a comment explaining its intended use.


const { ConfigurationStrategyInterface } = require('./strategy.interface')

Expand Down
4 changes: 2 additions & 2 deletions src/plugins/devices/configuration-strategies/factory.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
const { AbortSignal } = require('@azure/abort-controller')

const { AntMinerStrategy } = require('./ant-miner-strategy')
// const { AntMinerStrategy } = require('./ant-miner-strategy')
Copy link
Copy Markdown

@codereviewbot-ai codereviewbot-ai Bot Jan 24, 2025

Choose a reason for hiding this comment

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

The import statement for AntMinerStrategy is commented out. If this import is not needed, it should be removed to clean up the codebase and avoid confusion. If it is temporarily disabled, consider adding a comment explaining why it is commented out and under what conditions it should be re-enabled.

const { TcpConfigurationStrategy } = require('./tcp-strategy')
const { ConfigurationStrategyInterface } = require('./strategy.interface');

Expand All @@ -11,7 +11,7 @@ class ConfigurationStrategyFactory {
* @returns {Promise<ConfigurationStrategyInterface|null>}
*/
static async createStrategy(host, abort) {
const strategies = [TcpConfigurationStrategy, AntMinerStrategy]
const strategies = [TcpConfigurationStrategy]
for (const Strategy of strategies) {
try {
const strategy = new Strategy(host, abort)
Expand Down
12 changes: 9 additions & 3 deletions src/plugins/eth/web3Http.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,16 @@ const isRateLimitError = (response, payload) => {
return true
}

if (payload?.method === 'eth_call' && (response.error?.message?.includes('execution reverted') || response.error?.code === -32000)) {
// Some providers return execution reverted error for eth_call when rate limiting
if (
payload?.method === 'eth_call' &&
response.error?.message?.includes('execution reverted') &&
(response.error?.data === '' || response.error?.data === '0x' || response.error?.data === null || response.error?.data === undefined)
) {
return true
}


const message = response.error?.message?.toLowerCase()
if (!message) {
return false
Expand All @@ -21,9 +27,9 @@ const isRateLimitError = (response, payload) => {
message.includes('too many requests') ||
message.includes('rate limit exceeded') ||
message.includes('reached maximum qps limit') ||
message.includes('rate limit reached') ||
message.includes('rate limit reached') ||
message.includes("we can't execute this request") ||
message.includes("max message response size exceed") ||
message.includes("max message response size exceed") ||
message.includes("upgrade your plan") ||
message.includes("Failed to validate quota usage")
);
Comment on lines 27 to 35
Copy link
Copy Markdown

@codereviewbot-ai codereviewbot-ai Bot Feb 2, 2025

Choose a reason for hiding this comment

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

The method isRateLimitError uses multiple string includes checks to determine if the error message indicates rate limiting. This approach can be inefficient, especially if the error message is long or the number of substrings to check is large. Consider consolidating these checks into a single regular expression or using a more efficient string searching algorithm to improve performance.

Suggested Solution:

const rateLimitPatterns = /too many requests|rate limit exceeded|reached maximum qps limit|rate limit reached|we can't execute this request|max message response size exceed|upgrade your plan|Failed to validate quota usage/;
return rateLimitPatterns.test(message);

Expand Down
Loading