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 example #1248

Merged
merged 11 commits into from
Aug 9, 2022
Merged

Add payment-channel example #1248

merged 11 commits into from
Aug 9, 2022

Conversation

kanishkatn
Copy link
Contributor

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.

@HCastano HCastano changed the title add payment-channel example Add payment-channel example May 11, 2022
@HCastano HCastano added the A-examples [examples] Work item label May 11, 2022
@HCastano
Copy link
Contributor

@kanishkatn thanks for your PR! I'll try and spend some time this week on it 😃

Copy link
Contributor

@HCastano HCastano left a 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 returning Result's
    • This is more idiomatic Rust, and also allows for more ergonomic error handling from the perspective of other contracts as well as UIs

examples/payment_channel/Cargo.toml Outdated Show resolved Hide resolved
examples/payment_channel/Cargo.toml Outdated Show resolved Hide resolved
examples/payment_channel/Cargo.toml Outdated Show resolved Hide resolved
examples/payment_channel/Cargo.toml Outdated Show resolved Hide resolved
examples/payment_channel/README.md Outdated Show resolved Hide resolved
examples/payment_channel/lib.rs Outdated Show resolved Hide resolved
examples/payment_channel/lib.rs Outdated Show resolved Hide resolved
examples/payment_channel/lib.rs Outdated Show resolved Hide resolved
examples/payment_channel/lib.rs Outdated Show resolved Hide resolved
examples/payment_channel/lib.rs Outdated Show resolved Hide resolved
@kanishkatn
Copy link
Contributor Author

@HCastano Thank you for taking the time to review it. I'll make the changes in a day or two

@kanishkatn
Copy link
Contributor Author

kanishkatn commented May 29, 2022

Hi @HCastano , thanks for waiting. I have made the changes.

Regarding replacing panic with Result, I couldn't do it in the messages that terminates the contract. The method ink_env::test::assert_contract_termination used in tests, expects the function to return () and wasn't compiling for Result<()>. I couldn't find a way around it.

@paritytech-cicd-pr
Copy link

paritytech-cicd-pr commented May 29, 2022

🦑 📈 ink! Example Contracts ‒ Changes Report 📉 🦑

These are the results when building the examples/* contracts from this branch with cargo-contract 1.4.0-25bfb96 and comparing them to ink! master:

Δ Optimized Size Δ Used Gas Total Optimized Size Total Used Gas
accumulator 1.00 K
adder 2.04 K
contract-introspection 2.32 K
contract-terminate 0.92 K 275_000
contract-transfer 8.31 K 75_000
data-structures 1.73 K
delegate-calls 2.89 K 76_242
delegator 6.34 K 232_136
dns 8.81 K 225_000
erc1155 16.74 K 450_000
erc20 8.42 K 225_000
erc721 11.62 K 600_000
flipper 1.24 K 75_000
forward-calls 2.87 K 151_411
incrementer 1.14 K
mother 12.16 K
multisig 25.28 K 470_240
payment-channel +7.95 K 7.95 K
rand-extension 3.79 K 75_000
seal-code-hash 1.39 K
seal-ecdsa 1.71 K
set-code-hash 1.49 K 150_000
subber 2.06 K
trait-erc20 8.69 K 225_000
trait-flipper 0.97 K 75_000
trait-incrementer 1.12 K 150_000
updated-incrementer 9.72 K
upgradeable-flipper 1.48 K

Link to the run | Last update: Sat Jul 2 05:09:47 CEST 2022

@kanishkatn kanishkatn requested a review from HCastano June 6, 2022 08:25
Copy link
Contributor

@HCastano HCastano left a 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!

examples/payment_channel/Cargo.toml Outdated Show resolved Hide resolved
examples/payment-channel/Cargo.toml Outdated Show resolved Hide resolved
examples/payment-channel/Cargo.toml Outdated Show resolved Hide resolved
examples/payment-channel/lib.rs Show resolved Hide resolved
examples/payment-channel/lib.rs Outdated Show resolved Hide resolved
examples/payment-channel/lib.rs Outdated Show resolved Hide resolved
examples/payment-channel/lib.rs Outdated Show resolved Hide resolved
@@ -0,0 +1,557 @@
//! # Payment Channel
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's done.

examples/payment-channel/lib.rs Outdated Show resolved Hide resolved
examples/payment-channel/lib.rs Outdated Show resolved Hide resolved
@kanishkatn
Copy link
Contributor Author

@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!

@kanishkatn
Copy link
Contributor Author

Hey @HCastano , could we merge this? I believe it's ready to go in!

@HCastano
Copy link
Contributor

Hey @HCastano , could we merge this? I believe it's ready to go in!

Let me take another look at this PR this week 🔎

Copy link
Contributor

@HCastano HCastano left a 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!

examples/payment-channel/lib.rs Outdated Show resolved Hide resolved
examples/payment-channel/lib.rs Outdated Show resolved Hide resolved
examples/payment-channel/lib.rs Outdated Show resolved Hide resolved
examples/payment-channel/lib.rs Outdated Show resolved Hide resolved
examples/payment-channel/lib.rs Outdated Show resolved Hide resolved
examples/payment-channel/lib.rs Outdated Show resolved Hide resolved
examples/payment-channel/Cargo.toml Outdated Show resolved Hide resolved
examples/payment-channel/Cargo.toml Outdated Show resolved Hide resolved
examples/payment-channel/Cargo.toml Outdated Show resolved Hide resolved
@kanishkatn
Copy link
Contributor Author

I've made the changes. It's looking so clean now!!

@kanishkatn kanishkatn requested a review from HCastano July 29, 2022 13:34
Copy link
Contributor

@HCastano HCastano left a 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

@kanishkatn
Copy link
Contributor Author

Thank you for your time @HCastano !

@HCastano HCastano merged commit 7c24bfa into use-ink:master Aug 9, 2022
@ascjones ascjones mentioned this pull request Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-examples [examples] Work item
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants