-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore(lightning): Upgrade To Blocktank v2 API #1216
Conversation
@limpbrains, any thoughts on why this test might be failing? |
test expected QuickSetupReserveNote to be shown, but in reality QuickSetupBlocktankNote is shown. I've changed test to reflect it. Here is a video of the test run: Simulator.Screen.Recording.-.iPhone.14.Pro.-.2023-08-25.at.10.32.53.mp4 |
Looks like the total channel size (incoming + receiving) was exceeding the |
I no longer understand how to appease the testing gods as @limpbrains did, but it's at least ready for review. @pwltr |
Slider on the quick setup screen feels broken Simulator.Screen.Recording.-.iPhone.14.Pro.-.2023-08-26.at.09.21.50.mp4 |
Testing Gods are Pleased |
The custom setup flow is broken when selecting the maximum local balance: Simulator.Screen.Recording.-.iPhone.14.-.2023-08-31.at.18.17.02.mp4 |
Updates receiving package calculations. Updated blocktankPurchaseFee calculation.
Updated the calculations in this commit. Custom channels should get calculated correctly now no matter how many sats the user has. |
Updates receiving package calculations.
Updates channel fee value in CustomSetup.tsx. Updates useEffect in QuickSetup.tsx to set max value on load.
I'm surprised Blocktank allowed that channel purchase. The new api usually does not allow the client balance to exceed the lsp balance with the following error: In the latest commit, I've updated the Should be good to go for testing, just need to update the test script so that it passes with the new values for lightning onboarding. |
I don't think we want to default to 80% from a design perspecive. Also it doesn't resolve the issue with the unbalanced channel for me.
This still doesn't show the correct amount: |
We've made it pretty far in this PR. I think we should call a checkpoint here so we can get some additional testing in with the new api and apply needed fixes in separate PR's from there The Cost is the Blocktank fee. I don't believe we're able to calculate the cost of the transaction at this point. We might want to change the verbiage to "Blocktank Fee" or something similar. The 80% is part of a larger discussion especially as anchor channels emerge. |
This is working on the master branch. Unless I'm missing something I don't see the reason to introduce a regression here.
I don't understand the need for this change. The unbalanced channel issue is independent of the 80% limit. I'm unable to get a balanced channel with any spending amount. This really needs to be fixed otherwise BT v2 channels will be pretty much useless for receiving. |
Adds v1 to v2 order migration. Updates fee estimate on Custom Order flow.
The fee Cost discrepancy should be sorted as of the latest set of commits. |
Also increased the lsp balance when setting 0 sats on the clients end to something more usable. Should be good to go. |
Channel balancing seems better now, it's still defaulting to 80% spending balance in QuickSetup though. From my tests that doesn't affect balancing at all, and it's not in the design this way, so this should be reverted. Also there are a lot of leftover console logs. |
@pwltr Could you expand on this? Do you mean it's defaulting to a value greater than $1000 at 80%? |
Sets default client balance to 20%, if able, instead of 80%.
Gotcha. Updated here. Changed it from setting it to the max of 80% to 20%. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of nitpicks other than that LGTM
Description
Upgrades Bitkit to the Blocktank v2 API here.