Fix autowithdrawal logs #1073
Fix autowithdrawal logs #1073
Conversation
WalkthroughWalkthroughThe recent updates involve enhancements to the wallet management system, including the introduction of a new Changes
Assessment against linked issues
Possibly related issues
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (6)
Files skipped from review as they are similar to previous changes (6)
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| @@ -185,5 +169,5 @@ async function autowithdrawCLN ({ amount, maxFee }, { me, models, lnd }) { | |||
| expiry: 360 | |||
| }) | |||
|
|
|||
| return await createWithdrawal(null, { invoice: inv.bolt11, maxFee }, { me, models, lnd, autoWithdraw: true }) | |||
| return await createWithdrawal(null, { invoice: inv.bolt11, maxFee }, { me, models, lnd, walletId: wallet.id }) | |||
There was a problem hiding this comment.
Tip
Codebase Verification
The verification of the createWithdrawal function's integration with the walletId parameter across the codebase shows that in most cases, the walletId is correctly passed as part of the function's arguments. Specifically, in the worker/autowithdraw.js file, both occurrences (lines 139 and 172) include walletId within an object as the third parameter. This aligns with the expected integration pattern.
However, the calls in pages/api/lnwith.js (lines 69-71) do not include the walletId parameter. This might be intentional, depending on the function's design and the requirements of the API endpoint, but it does indicate that walletId is not universally integrated in all calls to createWithdrawal.
Given this information, it is recommended to review whether the absence of walletId in the API endpoint calls is appropriate or if it represents an oversight that needs correction.
Conclusion:
The createWithdrawal function is generally integrated with the walletId parameter correctly in the worker/autowithdraw.js file. However, its absence in some API calls may need further review to ensure consistency and correctness across the application.
Analysis chain
Confirm that the createWithdrawal function is properly integrated with the walletId parameter in all relevant calls.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Confirm integration of `walletId` in `createWithdrawal` function calls.
ast-grep --lang javascript --pattern $'createWithdrawal($_, $_, $_, $_)'
Length of output: 73
Script:
#!/bin/bash
# Search for all occurrences of `createWithdrawal` to understand its usage patterns.
ast-grep --lang javascript --pattern $'createWithdrawal($$$)'
Length of output: 720
There was a problem hiding this comment.
@ekzyis is this omission intentional?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Yes since the createWithdrawal call in pages/api/lnwith.js is not about an autowithdrawal
There was a problem hiding this comment.
I see. So once we ship this we'll remove the old procedure and this code will use the new one.
There was a problem hiding this comment.
Mhh, I see. So it's about when we remove create_withdrawal(TEXT, TEXT, BIGINT, BIGINT, TEXT, BOOLEAN) and old code is still calling it during the migration. This will fail since that function no longer exists. It wouldn't fail when there would be a default.
Therefore, I see following options:
a) Remove create_withdrawal(TEXT, TEXT, BIGINT, BIGINT, TEXT, BOOLEAN) and replace create_withdrawal(TEXT, TEXT, BIGINT, BIGINT, TEXT, BOOLEAN, INTEGER) with create_withdrawal(TEXT, TEXT, BIGINT, BIGINT, TEXT, BOOLEAN, INTEGER DEFAULT NULL) in the same future migration
b) Simply delete create_withdrawal(TEXT, TEXT, BIGINT, BIGINT, TEXT, BOOLEAN) since this will only break withdrawals while we're migrating which shouldn't take too long. This isn't backwards incompatible in a way that it will take down the site afaict. It's still an interesting problem to think about though.
c) Think more about versioned schemas like create_withdrawal_vX etc.
d) What are the reasons we're using postgres functions instead of inline JS code? Mostly historical since support for interactive transactions in Prisma was only made generally available in 4.7.0? I think there is a case to be made for interactive transactions instead of postgres functions.
I prefer option b)
There was a problem hiding this comment.
b sounds fine. Another option
e) have pages/api/lnwith use the new version by supplying a null wallet_id
There was a problem hiding this comment.
I think I am super confused now or I was super confused. I don't understand my own comment anymore:
So it's about when we remove
create_withdrawal(TEXT, TEXT, BIGINT, BIGINT, TEXT, BOOLEAN)and old code is still calling it during the migration
When we deploy the changes in this PR and the currently deployed code (1d154e) stops running, there will no longer be code that calls create_withdrawal(,,,,, BOOLEAN). There will only be code from this PR that calls create_withdrawal(,,,,, BOOLEAN, INTEGER) including the code in pages/api/lnwith. So we can safely remove create_withdrawal(,,,,, BOOLEAN) then.
But we actually will want to migrate from create_withdrawal(..., BOOLEAN, INTEGER) to just create_withdrawal(..., INTEGER). Is this the problem you are talking about? Even though I don't see how a INTEGER DEFAULT NULL fits in here.
There was a problem hiding this comment.
I just realized that page/api/lnwith doesn't have an independent call to create_with.. lol. Let's stop me from confusing you more.
Is this good to merge?
There was a problem hiding this comment.
This is as good as it gets
| -- AddForeignKey | ||
| ALTER TABLE "Withdrawl" ADD CONSTRAINT "Withdrawl_walletId_fkey" FOREIGN KEY ("walletId") REFERENCES "Wallet"("id") ON DELETE SET NULL ON UPDATE CASCADE; | ||
|
|
||
| CREATE OR REPLACE FUNCTION create_withdrawl(lnd_id TEXT, invoice TEXT, msats_amount BIGINT, msats_max_fee BIGINT, username TEXT, auto_withdraw BOOLEAN, wallet_id INTEGER) |
There was a problem hiding this comment.
I didn't replace the old function signature TEXT, TEXT, BIGINT, BIGINT, TEXT, BOOLEAN with TEXT, TEXT, BIGINT, BIGINT, TEXT, INTEGER since then the old code might be confused which function it should use if two signatures exist with the same amount of parameters during migration even though it should be obvious when looking at the JS types (boolean is not an integer).
But I neither trust JS nor Prisma to not fuck this up so to be safe, I added a new function signature.
worker/wallet.js
Outdated
| let walletName | ||
| if (dbWdrwl.wallet) { | ||
| const { wallet } = dbWdrwl.wallet | ||
| walletName = wallet.address ? 'walletLNAddr' : wallet.macaroon ? 'walletLND' : 'walletCLN' |
There was a problem hiding this comment.
This works, but dbWdrwl.wallet.type is probably be a better way to determine wallet types.
There was a problem hiding this comment.
Oh, yes. I should actually just use WalletType in the WalletLog table, too.
update: But that would be backwards incompatible 👀
I'll first use strings that would be compatible with WalletType. Then we can later migrate the column to actually use WalletType.
There was a problem hiding this comment.
🙊 I had a code review draft that mentioned that change in the wallet log PR but figured it was better to let you find it. I figured so long as it's not going to break, I can take this approach. :)
There was a problem hiding this comment.
This is actually a fun game to play 👀:
What did @huumn not tell me about this PR?
There was a problem hiding this comment.
This is why shipping MVPs is good because you begin thinking about what people might notice. Same goes for open source code. You begin simulating accountability.
There was a problem hiding this comment.
This reminds me of this:
I believe our brains create a literal simulation of our partner's mind that overtime converges towards their actual thought processes. In that way, we are feeling and thinking what they're feeling and thinking, and vice versa.
-- https://stacker.news/items/413755
Your analogy with startup hires being like marriages becomes more and more true over time
ekzyis
force-pushed
the
1069-fix-autowithdrawal-logs
branch
from
f573814
to
0a8b615
Compare
ekzyis
force-pushed
the
1069-fix-autowithdrawal-logs
branch
from
0a8b615
to
b715005
Compare
Description
Fix #1069 Based on #1072
Screenshots
Additional Context
autoWithdrawalcolumn even though it's obsolete with newwalletIdcolumnON DELETE SET NULLto not delete withdrawals if wallet was deletedChecklist
Summary by CodeRabbit