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

Add payment channel updated stream to ts rpc client #1803

Merged
merged 11 commits into from
Oct 5, 2023

Conversation

geoknee
Copy link
Contributor

@geoknee geoknee commented Oct 4, 2023

Closes #1770

  • Should we name it .onPaymentChannelUpdated ?

There is very little in the way of unit or integration tests setup at present. Therefore I have filed #1806, and we should make sure to cover this event stream in that work.

@netlify
Copy link

netlify bot commented Oct 4, 2023

👷 Deploy Preview for nitrodocs processing.

Name Link
🔨 Latest commit 1b547d2
🔍 Latest deploy log https://app.netlify.com/sites/nitrodocs/deploys/651f1d3ac9d2d800080b8b09

@geoknee
Copy link
Contributor Author

geoknee commented Oct 5, 2023

@bitwiseguy any opinion on this

Should we name it .onPaymentChannelUpdated ?

@bitwiseguy
Copy link
Contributor

bitwiseguy commented Oct 5, 2023

@bitwiseguy any opinion on this

Should we name it .onPaymentChannelUpdated ?

Sorry @geoknee I think I am missing some context. What exactly are you proposing that we name .onPaymentChannelUpdated? Can you point me to the relevant line(s) of code?

Comment on lines 108 to 116
/**
* PaymentChannelUpdated attaches a callback which is triggered when the channel with supplied ID is updated.
*
* @param objectiveId - The id objective to wait for
*/
PaymentChannelUpdated(
channelId: string,
callback: (info: PaymentChannelInfo) => void
): Promise<void>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's what I am wondering about the naming of.

Thinking about the usage, which of the following makes the most sense

nitroClient.PaymentChannelUpdated("0xabc",()=>{dothing()})
nitroClient.onPaymentChannelUpdated("0xabc",()=>{dothing()})

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok I got it. I don't have strong preference, but lean towards:

nitroClient.onPaymentChannelUpdate("0xabc",()=>{dothing()})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@geoknee geoknee merged commit 7558d3a into main Oct 5, 2023
3 checks passed
@geoknee geoknee deleted the ts-payment-channel-updated-events branch October 5, 2023 20:37
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.

Add PaymentChannelUpdated event stream to TS rpc client
2 participants