-
Notifications
You must be signed in to change notification settings - Fork 66
Add getSubscriptionStatus method #110
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
Conversation
✅ Heimdall Review Status
|
50189ed to
99e9ca5
Compare
99e9ca5 to
f5ff77c
Compare
f5ff77c to
44f34ab
Compare
fan-zhang-sv
left a comment
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.
LGTM!
| if (permission.chainId !== expectedChainId) { | ||
| // Determine if the subscription is on mainnet or testnet | ||
| const isSubscriptionOnMainnet = permission.chainId === CHAIN_IDS.base; | ||
| const isSubscriptionOnTestnet = permission.chainId === CHAIN_IDS.baseSepolia; | ||
|
|
||
| let errorMessage: string; | ||
| if (testnet && isSubscriptionOnMainnet) { | ||
| errorMessage = | ||
| 'The subscription was requested on testnet but is actually a mainnet subscription'; | ||
| } else if (!testnet && isSubscriptionOnTestnet) { | ||
| errorMessage = | ||
| 'The subscription was requested on mainnet but is actually a testnet subscription'; | ||
| } else { | ||
| // Fallback for unexpected chain IDs | ||
| errorMessage = `Subscription is on chain ${permission.chainId}, expected ${expectedChainId} (${testnet ? 'Base Sepolia' : 'Base'})`; | ||
| } | ||
|
|
||
| throw new Error(errorMessage); | ||
| } | ||
|
|
||
| if (permission.permission.token.toLowerCase() !== expectedTokenAddress) { | ||
| throw new Error( | ||
| `Subscription is not for USDC token. Got ${permission.permission.token}, expected ${expectedTokenAddress}` | ||
| ); | ||
| } |
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.
could abstract these to some internal subscription helper for reuseability
| * console.log(`Recurring amount: $${status.recurringAmount}`); | ||
| * ``` | ||
| */ | ||
| export async function getSubscriptionStatus( |
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.
I noticed there's no tests for this, are there plans to add any?
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.
will add!
| * @param currentTimestamp - Optional timestamp to use as "now" (defaults to current time) | ||
| * @returns The current period with start, end, and spend (0 for inferred periods) | ||
| */ | ||
| export function calculateCurrentPeriod( |
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.
Also should have a test
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.
will add!
What changed? Why?
Added a new
getSubscriptionStatusmethod to the payment interface that allows checking the status of a subscription created via thesubscribemethod.added
prepareChargewhich wraps existing utils after doing some validationsHow was this tested?
manually + unit
How can reviewers manually test these changes?
subscribemethodgetSubscriptionStatuswith the returned subscription IDDemo/screenshots