Removed whitelist codebase #357
Conversation
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
| @@ -56,8 +56,7 @@ const chain = { | |||
| titanLightningDashboard: process.env.TITAN_LIGHTNING_DASHBOARD || "https://lightning.titan.io", | |||
| defaultSellerCurrency: process.env.DEFAULT_SELLER_CURRENCY || 'BTC', | |||
|
|
|||
| bypassAuth: process.env.BYPASS_AUTH === "true", | |||
| sellerWhitelistUrl: process.env.SELLER_WHITELIST_URL || 'https://forms.gle/wEcAgppfK2p9YZ3g7' | |||
| bypassAuth: process.env.BYPASS_AUTH === "true" | |||
There was a problem hiding this comment.
The use of BYPASS_AUTH to potentially bypass authentication mechanisms poses a significant security risk. This feature should be handled with extreme caution, especially in production environments. It's recommended to:
- Ensure that this feature is disabled by default.
- Implement additional safeguards to prevent its misuse, such as logging and monitoring access when this feature is enabled.
- Consider removing or restricting this feature if not absolutely necessary.
| }) | ||
| .catch(error => { | ||
| setIsModalActive(false); | ||
| if (error.message == 'seller is not whitelisted') { | ||
| setShowSellerWhitelistForm(true); | ||
| return; | ||
| } | ||
| context.toast('error', error.message || error); | ||
| }) | ||
| .finally(() => { |
There was a problem hiding this comment.
Error Handling Improvement
The error handling in the catch block of the handleContractDeploy function could be improved by providing more specific error messages. Currently, the function uses error.message || error which might not always provide a clear indication of what went wrong, especially if error.message is undefined or not descriptive enough.
Recommendation:
- Ensure that the backend provides meaningful error messages.
- Implement a more sophisticated error handling strategy that could categorize errors (e.g., network issues, validation errors) and respond accordingly. This would improve the user's ability to understand what went wrong and how they might be able to fix it.
| showSuccess={false} | ||
| /> | ||
|
|
||
| <SellerWhitelistModal | ||
| isActive={showSellerWhitelistForm} | ||
| formUrl={formUrl} | ||
| close={() => { | ||
| setShowSellerWhitelistForm(false); | ||
| }} | ||
| /> | ||
|
|
||
| <AdjustProfitModal | ||
| isActive={showAdjustForm} | ||
| contracts={[...underProfitContracts]} |
There was a problem hiding this comment.
State Management in Modals
The AdjustProfitModal is controlled by a state variable isActive which is set based on the showAdjustForm state. This approach is straightforward but can be enhanced by encapsulating the modal's visibility and its state management within the modal component itself, rather than controlling it from the parent component.
Recommendation:
- Consider implementing a custom hook or a higher-order component that manages the visibility and state of modals. This would reduce the boilerplate in the parent component and improve the modularity and reusability of the modal components.
| ethCoinPrice: selectors.getRateEth(state), | ||
| btcCoinPrice: selectors.getRateBtc(state), | ||
| selectedCurrency: selectors.getSellerSelectedCurrency(state), | ||
| formUrl: selectors.getSellerWhitelistForm(state), | ||
| autoAdjustPriceInterval: selectors.getAutoAdjustPriceInterval(state), | ||
| autoAdjustContractPriceTimeout: selectors.getAutoAdjustContractPriceTimeout( | ||
| state |
There was a problem hiding this comment.
Performance and Maintainability Issue
The selectors used in mapStateToProps such as selectors.getRateEth, selectors.getRateBtc, and others might not be memoized. Non-memoized selectors can lead to unnecessary recalculations and re-renders, impacting performance negatively, especially when dealing with frequent state updates.
Recommendation:
Ensure that all selectors are memoized using libraries like Reselect to compute derived data, which can help in avoiding unnecessary computations and improve component performance.
| ethCoinPrice: selectors.getRateEth(state), | ||
| btcCoinPrice: selectors.getRateBtc(state), | ||
| selectedCurrency: selectors.getSellerSelectedCurrency(state), | ||
| formUrl: selectors.getSellerWhitelistForm(state), | ||
| autoAdjustPriceInterval: selectors.getAutoAdjustPriceInterval(state), | ||
| autoAdjustContractPriceTimeout: selectors.getAutoAdjustContractPriceTimeout( | ||
| state |
There was a problem hiding this comment.
Consistency and Error Handling Issue
The current implementation does not handle potential undefined or erroneous returns from selectors such as selectors.getRateEth and selectors.getRateBtc. This can lead to runtime errors or incorrect data being passed as props, which is critical in a financial context.
Recommendation:
Implement error handling and default values for each selector used in mapStateToProps. This could involve checking for undefined values and providing default values or handling errors gracefully to ensure the component remains functional and displays accurate information.
|
|
||
| export const getSellerDefaultCurrency = state => | ||
| state.config.chain.defaultSellerCurrency; |
There was a problem hiding this comment.
Error Handling Improvement
Currently, the selectors getSellerSelectedCurrency and getSellerDefaultCurrency do not handle cases where state.config or state.config.chain might be undefined. This can lead to runtime errors if the state structure is not as expected.
Recommendation:
Implement checks to ensure that the necessary parts of the state are defined before accessing their properties. For example:
export const getSellerSelectedCurrency = state => state.config ? state.config.sellerCurrency : undefined;
export const getSellerDefaultCurrency = state => state.config && state.config.chain ? state.config.chain.defaultSellerCurrency : undefined;This change will help prevent potential runtime errors and make the selectors more robust.
No description provided.