Skip to content
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

Fix ZapPlanner Subscription Budget Calculation and Interval Enforcement (Fixes #60) #61

Closed
wants to merge 3 commits into from

Conversation

adarsh-jha-dev
Copy link

This PR addresses the issue where the ZapPlanner subscription form did not correctly calculate the budget amount based on the selected interval. Additionally, it enforces a limitation on the subscription interval, ensuring it does not exceed one year. The changes made to the ConfirmSubscriptionForm component resolve these issues, improving the user experience when creating subscriptions.

Key Changes:

  • Calculate the budget in satoshis based on the selected interval (daily, weekly, monthly).
  • Enforce a maximum interval of one year.
  • Provide appropriate error messages and user feedback.

This PR fixes issue #60 and improves the overall functionality of the ZapPlanner subscription creation process.

// Calculate the number of satoshis based on the desired interval
const satoshis = getSatoshisForInterval(selectedInterval);

data.amount = satoshis;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, this is incorrect. We need to pass a max_amount field to https://github.com/getAlby/nostr-wallet-connect based on the amount and interval the user sets in ZapPlanner, not change the amount for the subscription. Please see the original ticket: #60

const selectedInterval = data.sleepDuration;

// Enforce that the interval should not be more than 1 year (365 days)
if (selectedInterval === "yearly") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use the duration in milliseconds. Otherwise this will let through e.g. 100 weeks or 1000000000 hours

@adarsh-jha-dev
Copy link
Author

I have implemented the changes , do let me know if there are any further issues

@rolznz
Copy link
Contributor

rolznz commented Oct 16, 2023

Hi @adarsh-jha-dev

I'm sorry, but this PR does not address the issue ticket, and your updates do not fix the bugs I already mentioned. Your changes do not at all integrate with the NWC budget system.

@adarsh-jha-dev
Copy link
Author

I hope the changes made now should have fixed the bug.

@rolznz
Copy link
Contributor

rolznz commented Oct 18, 2023

I will close this PR. I'm sorry but it does not in any way address the issue. In order to complete this task you need to create an Alby account and use Nostr Wallet Connect so you understand what the budgets mean and why we would want to add them here.

@rolznz rolznz closed this Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants