Fix Unknown command error when disabling Bolt12 offers on CLN
#1451
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes in this pull request involve modifying the URL used in the Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
backend/controllers/cln/offers.js (2)
86-86: Approve the URL change and suggest a minor improvement.The change from
/v1/disableOfferto/v1/disableoffercorrectly addresses the "Unknown command" error mentioned in the PR objectives and issue #1433. This modification aligns the URL with the gRPC method name used by Core Lightning (CLN).For consistency, consider extracting the base URL and endpoint into constants. This approach would make future updates easier and reduce the risk of similar typos.
Consider refactoring the URL construction as follows:
const BASE_URL = req.session.selectedNode.settings.lnServerUrl; const DISABLE_OFFER_ENDPOINT = '/v1/disableoffer'; options.url = `${BASE_URL}${DISABLE_OFFER_ENDPOINT}`;This change would improve maintainability and reduce the likelihood of similar issues in the future.
Line range hint
1-95: Consider a broader refactoring for consistency and maintainability.While the current change effectively addresses the immediate issue, there's an opportunity to improve the overall code structure to prevent similar problems in the future.
Consider the following suggestions:
- Create a constants file (e.g.,
apiEndpoints.js) to store all API endpoints:// apiEndpoints.js export const API_ENDPOINTS = { LIST_OFFERS: '/v1/listoffers', CREATE_OFFER: '/v1/offer', FETCH_INVOICE: '/v1/fetchinvoice', DISABLE_OFFER: '/v1/disableoffer', // Add other endpoints here };
- Update the
offers.jsfile to use these constants:import { API_ENDPOINTS } from '../../utils/apiEndpoints.js'; // In each function: options.url = `${req.session.selectedNode.settings.lnServerUrl}${API_ENDPOINTS.DISABLE_OFFER}`;This approach would:
- Centralize API endpoint management
- Reduce the risk of typos and inconsistencies
- Make it easier to update endpoints across the application
Additionally, consider creating a utility function for constructing API URLs to further reduce code duplication and improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- backend/controllers/cln/offers.js (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
backend/controllers/cln/offers.js (1)
Line range hint
1-95: Summary of the reviewThe change successfully addresses the "Unknown command" error when disabling Bolt12 offers on CLN. The modification is minimal and focused, reducing the risk of introducing new issues.
Key points:
- The URL change in the
disableOfferfunction resolves the reported issue.- Suggestions for improving code consistency and maintainability have been provided.
- No other significant issues were found in the file.
Overall, this change achieves the PR objectives and improves the functionality of the RTL application. The suggested refactoring would further enhance the code quality and prevent similar issues in the future.
ShahanaFarooqui
force-pushed
the
master
branch
from
91ad075 to
c26f60f
Compare
There was a problem hiding this comment.
@s373nZ Thanks for the PR. I also updated the changes in the TypeScript file (server/controllers/cln/offers.ts) so that it will reflect in the compiled JavaScript file (backend/controllers/cln/offers.js).
Compiling the TypeScript's server directory results in the final JavaScript backend directory. This can be done using the scripts npm run buildbackend or npm run watchbackend, both of which use the underlying tsc command.
ShahanaFarooqui
merged commit fc9150f
into
Ride-The-Lightning:Release-0.15.3
* Fix `Unknown command` error when disabling offers on CLN. ([#1443]) (#1451) * Add missing SSO options to config (#1455) * Fix for cln logic screen navigation (#1457) * Transactions destination address display fix (#1458) * cln delexpiredinvoices deprecation fix (#1459) * Read LN_IMPLEMENTATION from environment (#1460) * Add Fee Rate Information on Send Funds Modal (#1461) * Artifact script fix (#1464) * Add AMP toggle for LND Send Payments (#1466) --------- Co-authored-by: Se7enZ <[email protected]>
Fixes a typo to change camel case to lowercase, match CLN's gRPC method name.
Fixes #1433
Summary by CodeRabbit