Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78b4f92569
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/state/Autopilot.ts
Outdated
| log.w("Could not get node address from graph for autopilot peer", [error.message]); | ||
| } | ||
|
|
||
| return nodePubkey; |
There was a problem hiding this comment.
When getAutopilotPeerUri cannot resolve an address, it falls back to returning only the pubkey, but the open-channel path expects a pubkey@host peer string and splits on @ before calling connectPeer; this means autopilot attempts can fail for nodes that are not already resolvable in the local graph (for example early startup or custom nodes), leaving users unable to auto-open despite confirming the prompt.
Useful? React with 👍 / 👎.
src/state/Autopilot.ts
Outdated
|
|
||
| feeRate = Number.parseInt(prompt, 10); | ||
|
|
||
| if (feeRate === -1 || Number.isNaN(feeRate) || feeRate < 0 || feeRate > 100) { |
There was a problem hiding this comment.
The custom feerate validation allows 0 because it only checks feeRate < 0, even though the dialog says valid values are 1–100 sat/vB; entering 0 is then treated as falsy downstream and the channel open falls back to default fee selection, so the user's explicit custom-fee choice is silently ignored.
Useful? React with 👍 / 👎.
hsjoberg
force-pushed
the
blixt-autopilot
branch
2 times, most recently
from
eef2c5e to
9b258b0
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b258b08c1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/state/Autopilot.ts
Outdated
| transaction.numConfirmations < 1 || | ||
| getStoreState().onChain.transactionNotificationBlacklist.includes(transaction.txHash) |
There was a problem hiding this comment.
Filter logic in the subscribeTransactions callback only checks confirmations and the on-chain blacklist, so confirmed outgoing transactions can still call checkAutopilot(). Because the blacklist is populated only for positive-amount receives in OnChain.initialize (Number(transaction.amount) > 0), spending from wallet UTXOs can unexpectedly prompt users to open a new channel even though no inbound funds arrived, which is a behavior regression for the new “incoming transaction” autopilot flow.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I'm not sure there is any worry here because checkAutopilot() will check whether the autopilot feature is even activated, and so if there is any onchain funds available in the first place the user is forced to open a channel or disable autopilot.
| debugShowStartupInfo && | ||
| toast("syncedToChain time: " + (new Date().getTime() - start.getTime()) / 1000 + "s"); | ||
| await actions.setupAutopilot(getStoreState().settings.autopilotEnabled); | ||
| await getStoreActions().autopilot.checkAutopilot(); |
There was a problem hiding this comment.
Startup calls checkAutopilot() before waitForGraphSync(), but checkAutopilot() resolves the target peer via getNodeInfo and throws when the pubkey is not yet in the graph. On nodes that are chain-synced but still graph-syncing, this can fail the only startup autopilot attempt; if no new on-chain transaction arrives afterward, users with existing confirmed balance will never be prompted to open a channel until they manually retrigger it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is a good point, however it is possible to get node info while graph is being synced.
To get on-boarded quicker I will try first without graph synced, and then stay in a wait-loop for graph to be synced.
hsjoberg
force-pushed
the
blixt-autopilot
branch
from
9b258b0 to
9d3952d
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d3952d81b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/state/Autopilot.ts
Outdated
| transaction.numConfirmations < 1 || | ||
| getStoreState().onChain.transactionNotificationBlacklist.includes(transaction.txHash) |
There was a problem hiding this comment.
subscribeTransactions currently triggers checkAutopilot() for any confirmed transaction that is not blacklisted, but it never checks that the tx is incoming (amount > 0). In practice this means confirmed outgoing spends can also trigger the auto-open flow, and because outgoing txids are not added to transactionNotificationBlacklist by onChain.initialize (that path only blacklists positive amounts), users can get repeated autopilot prompts on their own spend confirmations.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
amount in lnrpc.Transaction is always positive (EDIT: this is wrong).
Furthermore, Onchain store's sendCoins() adds the wallet's own transactions to the blacklist before `subscribeTransactions(), causing it to get caught in the blacklist.
src/state/Autopilot.ts
Outdated
| const prompt = await Alert.promisePromptCallback( | ||
| "Set custom feerate", | ||
| "Enter the feerate in sat/vB for opening the channel", | ||
| ); |
There was a problem hiding this comment.
When the user selects Yes and set custom feerate, this code awaits Alert.promisePromptCallback(...) after setting autopilotPrompting=true; if the prompt is dismissed/canceled, that promise can remain unresolved for function callbacks (see cancel/dismiss branches in src/utils/alert.ts), so the thunk never reaches its finally and autopilotPrompting stays true indefinitely. After that, all future checkAutopilot() calls are skipped as "already prompting" until restart.
Useful? React with 👍 / 👎.
hsjoberg
force-pushed
the
blixt-autopilot
branch
from
9d3952d to
09bf94f
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09bf94f43c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| transaction.numConfirmations < 1 || | ||
| getStoreState().onChain.transactionNotificationBlacklist.includes(transaction.txHash) | ||
| ) { |
There was a problem hiding this comment.
This transaction callback triggers checkAutopilot() for any confirmed tx that is not blacklisted, but it never verifies that the tx is incoming. In LND transaction streams, confirmed outgoing spends also pass this condition, so users can get channel-open prompts after sending on-chain funds (as long as they still have enough balance), which is inconsistent with the intended "incoming transaction" trigger path. Add an incoming-amount guard (like the on-chain notifier’s amount > 0 check) before invoking autopilot.
Useful? React with 👍 / 👎.
hsjoberg
force-pushed
the
blixt-autopilot
branch
2 times, most recently
from
7a745d2 to
7fbd540
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7fbd540c5a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/state/Autopilot.ts
Outdated
| if ( | ||
| transaction.numConfirmations < 1 || | ||
| getStoreState().onChain.transactionNotificationBlacklist.includes(transaction.txHash) || | ||
| Number(transaction.amount) > 0 |
There was a problem hiding this comment.
The transaction guard currently exits when Number(transaction.amount) > 0, but in this codebase positive on-chain amounts represent received funds (see the incoming-notification check in src/state/OnChain.ts), so this inverts the autopilot trigger. In practice, confirmed deposits are skipped while non-positive (outgoing) transactions can invoke checkAutopilot, which breaks the new auto-open flow and can prompt users at the wrong time. Reverse this amount check so incoming confirmed transactions are the ones that trigger autopilot.
Useful? React with 👍 / 👎.
hsjoberg
force-pushed
the
blixt-autopilot
branch
from
7fbd540 to
d36a9b8
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d36a9b8332
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/windows/Settings/Settings.tsx
Outdated
| changeAutopilotEnabled(!autopilotEnabled); | ||
| setupAutopilot(!autopilotEnabled); | ||
| if (!autopilotEnabled) { | ||
| checkAutopilot(); |
There was a problem hiding this comment.
changeAutopilotEnabled is an async thunk that persists to storage before it updates settings.autopilotEnabled, but checkAutopilot() is called immediately afterward without awaiting that update. In the enable flow (autopilotEnabled was false), checkAutopilot can read the stale disabled state and exit at its early guard, so users with sufficient on-chain funds may not get the expected prompt until a later trigger (e.g., another transaction or restart).
Useful? React with 👍 / 👎.
hsjoberg
force-pushed
the
blixt-autopilot
branch
from
d36a9b8 to
2886f1b
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2886f1b660
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/windows/Settings/Settings.tsx
Outdated
| changeAutopilotEnabled(!autopilotEnabled); | ||
| setupAutopilot(!autopilotEnabled); | ||
| if (!autopilotEnabled) { | ||
| await checkAutopilot(); |
There was a problem hiding this comment.
When the user enables autopilot here, changeAutopilotEnabled is fired but not awaited, and checkAutopilot() is called immediately afterward. Since checkAutopilot gates on settings.autopilotEnabled, it can read the old false value and return early, so the expected prompt to open a channel does not appear until a later trigger (e.g., a new transaction or manual force). This is a regression from the previous flow, which passed the intended enable state directly.
Useful? React with 👍 / 👎.
| { text: "Yes" }, | ||
| { text: "Yes and set custom feerate" }, | ||
| { text: "No and disable autopilot", style: "cancel" }, |
There was a problem hiding this comment.
This adds two non-cancel choices to Alert.promiseAlert, but the web implementation (WebAlert.alert) is still backed by window.confirm, which only supports a boolean choice and resolves the first non-cancel button. In web builds, users can never select Yes and set custom feerate, so that new branch is unreachable and custom fee entry is effectively broken on that platform.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The web implementation is just a demo and in the future HelperWindows/Prompt.tsx can be used instead.
hsjoberg
force-pushed
the
blixt-autopilot
branch
from
2886f1b to
7d57843
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d57843499
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| debugShowStartupInfo && | ||
| toast("syncedToChain time: " + (new Date().getTime() - start.getTime()) / 1000 + "s"); | ||
| await actions.setupAutopilot(getStoreState().settings.autopilotEnabled); | ||
| await getStoreActions().autopilot.checkAutopilot(); |
There was a problem hiding this comment.
Replacing the startup call with autopilot.checkAutopilot() removed the only production path that called autopilotModifyStatus, so existing daemons that were already running with lnd autopilot enabled (from previous app versions) are no longer forced off. In the SERVER_ACTIVE/already-running-daemon flow, this means lnd can continue opening channels in the background without the new prompt flow, and even after the user toggles autopilot off in settings, because only app state is changed now.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This isn't true because autopilot always starts as inactive, and modifyStatus gRPC call isn't a persistent change (it's only for the current runtime).
blixt-wallet/src/state/index.ts
Lines 741 to 742 in cc8249e
1 participant
This PR removes the usage of lnd's internal autopilot in favor of our own. We introduce the Autopilot easy-peasy store which will listen to incoming onchain transactions and check
This way we have have more control over how it's supposed to function -- for example we now prompt the user whether they want to open a channel, and can also choose to set a custom fee rate.