Skip to content
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.

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

Feat/eth http poll #308

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from
Open

Feat/eth http poll #308

wants to merge 8 commits into from

Conversation

bohdan-titan
Copy link
Contributor

No description provided.

Comment on lines 346 to 348
const logout = async (data) => {
return cleanupDb();
};

Choose a reason for hiding this comment

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

The logout function is calling cleanupDb() without passing any arguments or handling the result or potential errors. If cleanupDb is intended to clean up database resources specific to the user session, it should likely be passed the necessary user/session data to perform its task correctly. Additionally, there is no error handling, which could lead to uncaught exceptions if cleanupDb fails. It is recommended to pass the necessary data to cleanupDb and implement proper error handling to ensure resources are cleaned up safely and errors are managed appropriately.

Comment on lines 408 to 412
getMarketplaceFee,
isProxyPortPublic,
stopProxyRouter,
getContractHistory,
};

Choose a reason for hiding this comment

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

The exported object at the end of the file includes stopProxyRouter, which is not defined in the provided code fragment. This could either be a mistake or the function is defined elsewhere in the file. If stopProxyRouter is not defined or is not intended to be exported, it should be removed from the export object to prevent runtime errors when attempting to access an undefined function. If it is defined elsewhere, ensure that it is correctly implemented and intended to be part of the public API of this module.

Comment on lines 22 to 24
function startCore({ chain, core, config: coreConfig }, webContent) {
logger.verbose(`Starting core ${chain}`);
const { emitter, events, api } = core.start(coreConfig);

Choose a reason for hiding this comment

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

The startCore function is initiating a core without any error handling mechanism. If the core.start method throws an exception or fails to start for any reason, it could cause the application to crash or enter an undefined state. It is important to implement error handling to ensure the application can recover gracefully from failures during the core startup process.

Recommended solution: Wrap the core startup logic in a try-catch block and handle any potential exceptions appropriately. This could include logging the error, retrying the startup process, or notifying the user of the failure.

Comment on lines 46 to 47
const contractsToShow = contracts.filter(
x => x.buyer === address && x.seller !== address

Choose a reason for hiding this comment

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

The contracts.filter operation is directly filtering contracts based on the equality of the buyer property to the address variable and the inequality of the seller property to the same address variable. This logic assumes that a contract cannot have the same address for both buyer and seller, which might be a valid business rule. However, if it is possible for a contract to have the same address for both buyer and seller, this filter will exclude such contracts. Ensure that this business rule is intentional and correctly implemented. If contracts with the same buyer and seller are valid and should be shown, the filter condition needs to be adjusted accordingly.

@@ -95,8 +94,7 @@ function BuyerHub({
const [showHashrateModal, setShowHashrateModal] = useState(false);
const [contactToShowHashrate, setContactToShowHashrate] = useState();

const contractsWithHistory = contracts.filter(c => c.history.length);
const showHistory = contractsWithHistory.length;
const hasContractsWithHistory = true;

Choose a reason for hiding this comment

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

The variable hasContractsWithHistory is set to true and never changed, which suggests that it is a constant value indicating that there are always contracts with history. If this is not the case and the presence of contracts with history should be dynamically determined, this variable should be calculated based on the actual contract data. If it is indeed a constant and the application always has contracts with history, consider removing this variable and directly using the true value where needed, or explaining its purpose if it serves as a placeholder for future dynamic behavior.

Comment on lines +18 to +48
const [historyContracts, setHistory] = useState({});
const [isLoading, setLoading] = useState(false);
useEffect(() => {
if (!isActive) {
return;
}
if (contracts.length) {
setLoading(true);
}
let loaded = 0;
for (let i = 0; i < contracts.length; i += 1) {
const c = contracts[i];
client
.getContractHistory({ contractAddr: c.id, walletAddress: address })
.then(history => {
if (history.length > 0) {
setHistory(prev => ({
...prev,
[c.id]: history
}));
}
})
.catch(err => {})
.finally(() => {
loaded += 1;
if (loaded === contracts.length) {
setLoading(false);
}
});
}
}, [isActive]);

Choose a reason for hiding this comment

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

The useEffect hook is used to fetch contract history data, but it does not handle the asynchronous nature of the data fetching properly. The loop inside the useEffect hook initiates multiple asynchronous calls without waiting for them to complete before initiating the next one. This can lead to race conditions where the state updates from these calls can overwrite each other, resulting in inconsistent UI updates.

To resolve this issue, consider using Promise.all to wait for all the contract history data to be fetched before updating the state. This will ensure that all the data is loaded and the state is updated consistently. Additionally, handle the case where the component unmounts before the asynchronous calls complete to avoid setting state on an unmounted component.

}));
}
})
.catch(err => {})

Choose a reason for hiding this comment

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

The .catch block in the promise chain is empty, which means that any errors that occur during the fetching of contract history are silently ignored. This can make debugging difficult and leave the user without any indication that an error has occurred.

To improve error handling, implement proper error logging within the .catch block and provide user feedback, such as displaying an error message, to indicate that something went wrong during the data fetching process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants