Skip to content

Feat/eth http poll #308

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

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

Feat/eth http poll #308

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c4d79e9
integrate http polling for contracts
alex-sandrk Oct 24, 2023
1a1b34b
transactions pool
alex-sandrk Oct 26, 2023
e98ce6b
Fixed mapping
bohdan-titan Nov 7, 2023
b182f89
Removed some comments
bohdan-titan Nov 13, 2023
67f4978
update polling
alex-sandrk Dec 20, 2023
695f17a
merge dev
alex-sandrk Dec 20, 2023
f102336
refactor: code review fixes
alex-sandrk Dec 20, 2023
d445d27
restore BuyerHub history feature
alex-sandrk Dec 27, 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
19 changes: 14 additions & 5 deletions public/main/client/handlers/single-core.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,9 +271,12 @@ const getMarketplaceFee = async function (data, { api }) {
return api.contracts.getMarketplaceFee(data);
};

function refreshAllContracts({ }, { api }) {
const walletId = wallet.getAddress().address;
return api.contracts.refreshContracts(null, walletId);
function refreshAllContracts({}, { api }) {
return api.contracts.refreshContracts();
}

function startWatchingContracts({}, { api }) {
return api.contracts.startWatching();
}

function refreshTransaction({ hash, address }, { api }) {
Expand Down Expand Up @@ -336,6 +339,10 @@ const getLocalIp = async ({ }, { api }) => api["proxy-router"].getLocalIp();

const isProxyPortPublic = async (data, { api }) => api["proxy-router"].isProxyPortPublic(data);

const getContractHistory = async (data, { api }) => {
return api.contracts.getContractHistory(data);
}

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

@codereviewbot-ai codereviewbot-ai bot Dec 27, 2023

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.

Expand All @@ -361,12 +368,13 @@ const revealSecretPhrase = async (password) => {
}

function getPastTransactions({ address, page, pageSize }, { api }) {
return api.explorer.getPastCoinTransactions(0, undefined, address, page, pageSize);
return api.explorer.syncTransactions(0, undefined, page, pageSize, address);
}

module.exports = {
// refreshAllSockets,
refreshAllSockets,
refreshAllContracts,
startWatchingContracts,
purchaseContract,
createContract,
cancelContract,
Expand Down Expand Up @@ -400,4 +408,5 @@ module.exports = {
getMarketplaceFee,
isProxyPortPublic,
stopProxyRouter,
getContractHistory,
};
Comment on lines 408 to 412
Copy link

@codereviewbot-ai codereviewbot-ai bot Dec 27, 2023

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.

15 changes: 11 additions & 4 deletions public/main/client/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@ const {
} = require("./handlers/single-core");

const { runProxyRouter, isProxyRouterHealthy } = require("./proxyRouter");

let interval;

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

@codereviewbot-ai codereviewbot-ai bot Dec 27, 2023

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.

Expand All @@ -35,6 +37,7 @@ function startCore({ chain, core, config: coreConfig }, webContent) {
"transactions-scan-finished",
"contracts-scan-started",
"contracts-scan-finished",
"contracts-updated",
'contract-updated',
);

Expand Down Expand Up @@ -65,10 +68,10 @@ function startCore({ chain, core, config: coreConfig }, webContent) {
return api.explorer
.syncTransactions(
0,
address,
(number) => storage.setSyncBlock(number, chain),
'latest',
page,
pageSize
pageSize,
address
)
.then(function () {
send("transactions-scan-finished", { success: true });
Expand All @@ -91,7 +94,11 @@ function startCore({ chain, core, config: coreConfig }, webContent) {
});
}

emitter.on("open-wallet", syncTransactions);
emitter.on("open-wallet", (props) => {
syncTransactions(props);
api.contracts.startWatching({});
api.explorer.startWatching({ walletAddress: props.address });
});

emitter.on("wallet-error", function (err) {
logger.warn(
Expand Down
4 changes: 3 additions & 1 deletion public/main/client/subscriptions/single-core.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ const listeners = {
"login-submit": handlers.onLoginSubmit,
// 'refresh-all-sockets': handlers.refreshAllSockets,
"refresh-all-contracts": handlers.refreshAllContracts,
"start-watching-contracts": handlers.startWatchingContracts,
"refresh-all-transactions": handlers.refreshAllTransactions,
"refresh-transaction": handlers.refreshTransaction,
"get-gas-limit": handlers.getGasLimit,
Expand All @@ -31,7 +32,8 @@ const listeners = {
"stop-proxy-router": handlers.stopProxyRouter,
"claim-faucet": handlers.claimFaucet,
'get-private-key': handlers.getAddressAndPrivateKey,
"get-marketplace-fee": handlers.getMarketplaceFee
"get-marketplace-fee": handlers.getMarketplaceFee,
"get-contract-history": handlers.getContractHistory,
};

let coreListeners = {};
Expand Down
7 changes: 6 additions & 1 deletion src/client/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,10 @@ const createClient = function(createStore) {
'refresh-all-contracts',
120000
),
startWatchingContracts: utils.forwardToMainProcess(
'start-watching-contracts',
120000
),
onOnboardingCompleted: utils.forwardToMainProcess('onboarding-completed'),
recoverFromMnemonic: utils.forwardToMainProcess('recover-from-mnemonic'),
getTokenGasLimit: utils.forwardToMainProcess('get-token-gas-limit'),
Expand Down Expand Up @@ -168,7 +172,8 @@ const createClient = function(createStore) {
claimFaucet: utils.forwardToMainProcess('claim-faucet', 750000),
getCustomEnvValues: utils.forwardToMainProcess('get-custom-env-values'),
setCustomEnvValues: utils.forwardToMainProcess('set-custom-env-values'),
getContractHashrate: utils.forwardToMainProcess('get-contract-hashrate')
getContractHashrate: utils.forwardToMainProcess('get-contract-hashrate'),
getContractHistory: utils.forwardToMainProcess('get-contract-history')
};

const api = {
Expand Down
15 changes: 8 additions & 7 deletions src/components/contracts/BuyerHub.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,7 @@ function BuyerHub({
address,
client,
contractsRefresh,
allowSendTransaction,
...props
allowSendTransaction
}) {
const contractsToShow = contracts.filter(
x => x.buyer === address && x.seller !== address
Comment on lines 46 to 47
Copy link

@codereviewbot-ai codereviewbot-ai bot Dec 27, 2023

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.

Expand Down Expand Up @@ -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;
Copy link

@codereviewbot-ai codereviewbot-ai bot Dec 27, 2023

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.

const onHistoryOpen = () => setIsHistoryModalOpen(true);

return (
Expand All @@ -106,10 +104,12 @@ function BuyerHub({
address={address}
copyToClipboard={copyToClipboard}
>
<HistoryBtn disabled={!showHistory} onClick={onHistoryOpen}>
<HistoryBtn disabled={!hasContractsWithHistory} onClick={onHistoryOpen}>
<span
style={{ display: 'flex' }}
data-rh={showHistory ? null : `You have no purchase history`}
data-rh={
hasContractsWithHistory ? null : `You have no purchase history`
}
>
<IconHistory style={{ display: 'inline-block' }} /> History
</span>
Expand All @@ -132,7 +132,8 @@ function BuyerHub({

<HistoryModal
isActive={isHistoryModalOpen}
historyContracts={contractsWithHistory}
contracts={contracts}
address={address}
close={() => {
setIsHistoryModalOpen(false);
}}
Expand Down
1 change: 1 addition & 0 deletions src/components/contracts/modals/CreateContractModal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ function CreateContractModal(props) {
if (!isActive) {
return <></>;
}

const timeField = register('time', {
required: true,
min: 24,
Expand Down
50 changes: 42 additions & 8 deletions src/components/contracts/modals/HistoryModal/HistoryModal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,18 +10,49 @@ import {
} from '../CreateContractModal.styles';
import HistoryRow from './HistoryRow';
import { withClient } from '../../../../store/hocs/clientContext';
import { lmrDecimals } from '../../../../utils/coinValue';
import Spinner from '../../../common/Spinner';

function HistroyModal(props) {
const { isActive, close, historyContracts, client } = props;
const { isActive, close, client, contracts, address } = props;

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 => {})
Copy link

@codereviewbot-ai codereviewbot-ai bot Dec 27, 2023

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.

.finally(() => {
loaded += 1;
if (loaded === contracts.length) {
setLoading(false);
}
});
}
}, [isActive]);
Comment on lines +18 to +48
Copy link

@codereviewbot-ai codereviewbot-ai bot Dec 27, 2023

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.


const handleClose = e => {
close(e);
};
const handlePropagation = e => e.stopPropagation();

const history = historyContracts
.map(hc => hc.history)
const history = Object.values(historyContracts)
.flat()
.map(h => {
return {
Expand All @@ -48,10 +79,7 @@ function HistroyModal(props) {
}

const rowRenderer = historyContracts => ({ key, index, style }) => (
<HistoryRow
key={historyContracts[index].id}
contract={historyContracts[index]}
/>
<HistoryRow key={`${index}`} contract={historyContracts[index]} />
);

return (
Expand All @@ -61,6 +89,12 @@ function HistroyModal(props) {
<TitleWrapper>
<Title>Purchase history</Title>
</TitleWrapper>
{isLoading && !history.length && (
<Subtitle>
Loading... <Spinner size="16px"></Spinner>
</Subtitle>
)}
{!isLoading && !history.length && <Subtitle>No history found</Subtitle>}
<AutoSizer width={400}>
{({ width, height }) => (
<RVList
Expand Down
3 changes: 3 additions & 0 deletions src/store/hocs/withContractsState.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ const withContractsState = WrappedComponent => {
contractsRefresh = (force = false) => {
const now = parseInt(Date.now() / 1000, 10);
const timeout = 15; // seconds
if (this.props.syncStatus === 'syncing') {
return;
}
if (
this.props.contractsLastUpdatedAt &&
now - this.props.contractsLastUpdatedAt < timeout &&
Expand Down
5 changes: 2 additions & 3 deletions src/store/reducers/contracts.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,12 @@ const reducer = handleActions(
};
},

'contract-updated': (state, { payload }) => {
'contracts-updated': (state, { payload }) => {
const idContractMap = keyBy(payload.actives, 'id');

return {
...state,
actives: { ...state.actives, ...idContractMap },
lastUpdated: parseInt(Date.now() / 1000, 10)
actives: { ...state.actives, ...idContractMap }
};
},

Expand Down
88 changes: 31 additions & 57 deletions src/store/reducers/wallet.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,51 +26,24 @@ export const initialState = {
* Should filter transactions without receipt if we received ones
*/
const mergeTransactions = (stateTxs, payloadTxs) => {
const txWithReceipts = payloadTxs.filter(tx => tx.receipt);
const newStateTxs = { ...stateTxs };

for (const tx of txWithReceipts) {
const key = `${tx.transaction.hash}_${tx.receipt.tokenSymbol || 'ETH'}`;
const oldStateTx = stateTxs[key];

const isDifferentLogIndex =
oldStateTx?.transaction?.logIndex &&
tx?.transaction?.logIndex &&
oldStateTx?.transaction?.logIndex !== tx?.transaction?.logIndex; // means that this is a second transaction within the same hash

if (oldStateTx && !isDifferentLogIndex) {
continue;
}
newStateTxs[key] = tx;
// contract purchase emits 2 transactions with the same hash
// as of now we merge corresponding amount values. Temporary fix, until refactoring trasactions totally

// we sum transaction value if it is transfers within the same transaction, but with different logIndex
// TODO: display both transactions in the UI either separately or as a single one with two outputs
if (oldStateTx && isDifferentLogIndex) {
if (
newStateTxs[key].transaction.value &&
oldStateTx.transaction.logIndex !== tx.transaction.logIndex
) {
newStateTxs[key].transaction.value = String(
Number(oldStateTx.transaction.value) + Number(tx.transaction.value)
);
}

if (newStateTxs[key].transaction.input.amount) {
newStateTxs[key].transaction.input.amount = String(
Number(oldStateTx.transaction.input.amount) +
Number(tx.transaction.input.amount)
);
}

if (newStateTxs[key].receipt.value) {
newStateTxs[key].receipt.value = String(
Number(oldStateTx.receipt.value) + Number(tx.receipt.value)
);
const txs = Object.values(payloadTxs).filter(x => typeof x == 'object');

for (const tx of txs) {
const flattenObjects = tx.transfers.map(x => ({
...tx,
...x,
transfers: undefined
}));
for (const obj of flattenObjects) {
if (obj.amount == 0) {
continue;
}
const key = `${obj.txhash}_${obj.token || 'ETH'}`;
newStateTxs[key] = obj;
}
}

return newStateTxs;
};

Expand Down Expand Up @@ -114,22 +87,23 @@ const reducer = handleActions(
}
}),

'token-transactions-changed': (state, { payload }) => ({
...state,
token: {
...state.token,
transactions: mergeTransactions(
state.token.transactions,
payload.transactions
)
}
}),

'transactions-next-page': (state, { payload }) => ({
...state,
hasNextPage: payload.hasNextPage,
page: payload.page
}),
'token-transactions-changed': (state, { payload }) => {
return {
...state,
token: {
...state.token,
transactions: mergeTransactions(state.token.transactions, payload)
}
};
},

'transactions-next-page': (state, { payload }) => {
return {
...state,
hasNextPage: payload.hasNextPage,
page: payload.page
};
},

'token-state-changed': (state, { payload }) => ({
...state,
Expand Down
Loading