-
Notifications
You must be signed in to change notification settings - Fork 430
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 example #1248
Conversation
@kanishkatn thanks for your PR! I'll try and spend some time this week on it 😃 |
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.
Good start!
I haven't gone through everything just yet, but I think there's enough comments to get you going before I circle back to this.
A few other things
- You should merge
master
into your branch, there's been a few changes since you opened this (sorry about that!) - Instead of
panic!
-ing, try returningResult
's- This is more idiomatic Rust, and also allows for more ergonomic error handling from the perspective of other contracts as well as UIs
@HCastano Thank you for taking the time to review it. I'll make the changes in a day or two |
Hi @HCastano , thanks for waiting. I have made the changes. Regarding replacing |
🦑 📈 ink! Example Contracts ‒ Changes Report 📉 🦑These are the results when building the
Link to the run | Last update: Sat Jul 2 05:09:47 CEST 2022 |
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'll slowly keep chipping away at this, sorry for taking so long!
@@ -0,0 +1,557 @@ | |||
//! # Payment Channel |
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.
Thanks for adding this. Can you also wrap everything here to 90 characters?
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.
It's done.
@HCastano No rush. I've been a bit swamped at work as well. I've resolved all the changes mentioned in the review comments. I'm learning so much from this. Thank you! |
Hey @HCastano , could we merge this? I believe it's ready to go in! |
Let me take another look at this PR this week 🔎 |
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.
Some small things left, but I think we can merge soon!
I've made the changes. It's looking so clean now!! |
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.
Thanks for adding this example! Sorry it took so long.
The CI failures aren't related to your code, we've been having some issues with our pipelines recently. So if we don't merge this it's because we're waiting on the CI fixes
Thank you for your time @HCastano ! |
I've been working on implementing payment-channel contract and felt it could go into the examples here. I've basically implemented it by referring to this post which does it in Solidity.
This is my first PR. I've followed the contributing guidelines. Happy to make any changes if needed.