Conversation
|
|
Is this ready for review or are there any blockers that I can help with? |
|
I haven't had the time yet to verify if I didn't produce any slop. But maybe you can give it a quick look if the direction is what you were looking for, and afterwards I will give it a de-slopping. |
|
@codex please trigger a manual 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: aa707840f1
ℹ️ 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".
| op.method = 'default'; | ||
| } | ||
| if (!op.methodData) { | ||
| op.methodData = {}; |
There was a problem hiding this comment.
The v10 IndexedDB upgrade populates op.methodData, but the repository now reads row.methodDataJson and immediately JSON.parses it when hydrating send operations. For databases upgraded from older versions, legacy rows keep methodDataJson unset, so reading those operations can throw at runtime (JSON.parse(undefined)), which can break startup recovery and any send-operation queries until rows are rewritten.
Useful? React with 👍 / 👎.
| // Use P2PK type for send outputs, custom for keep outputs | ||
| // Note: P2PK type doesn't accept custom data - the wallet generates P2PK-locked outputs | ||
| const outputConfig: OutputConfig = { | ||
| send: { type: 'p2pk', options: { pubkey } }, |
There was a problem hiding this comment.
This execute path tells the wallet to generate P2PK send outputs instead of using the serialized outputData.send prepared earlier, but pending/finalization logic still derives send secrets from that stored output data. In P2PK sends, those secrets diverge from the actual minted proofs, so pending checks and rollback/recovery can target non-existent proofs and leave claimed operations stuck in pending.
Useful? React with 👍 / 👎.
Egge21M
left a comment
Egge21M
left a comment
There was a problem hiding this comment.
Awesome! Thank you for taking this on. I love the direction and generally this is already really good! I noticed a few things here and there, please take a look at those. And I can do another full review once the PR is ready-for-review
🥜
| } | ||
|
|
||
| // Try exact match first (no swap needed) | ||
| const exactProofs = wallet.selectProofsToSend(availableProofs, amount, false); |
There was a problem hiding this comment.
ProofService exposes a method for coin selection called: ProofService.selectProofsToSend
| (p: CoreProof) => sendSecrets.includes(p.secret) && p.state === 'inflight', | ||
| ); | ||
|
|
||
| if (sendProofs.length > 0) { |
There was a problem hiding this comment.
Pre exisiting bug / edge case: A pending operation indicates that the proofs were sent, but not claimed yet. If the watcher did not update us correctly, this might be a false assumption (e.g. proofs are already claimed by the receiver, but we don't know it yet).
We should probably include a spent-check here
| private readonly mintService: MintService; | ||
| private readonly walletService: WalletService; | ||
| private readonly eventBus: EventBus<CoreEvents>; | ||
| private readonly handlerProvider?: SendHandlerProvider; |
|
@gandlafbtc are you still working on this? |
2 participants
#47
#77