-
Notifications
You must be signed in to change notification settings - Fork 5
32 update payment channel #41
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Minh Huy Tran <[email protected]>
Signed-off-by: Minh Huy Tran <[email protected]>
Signed-off-by: Minh Huy Tran <[email protected]>
…+ remove simple client from active CI Signed-off-by: Minh Huy Tran <[email protected]>
Signed-off-by: Minh Huy Tran <[email protected]>
Signed-off-by: Minh Huy Tran <[email protected]>
Signed-off-by: Minh Huy Tran <[email protected]>
Signed-off-by: Minh Huy Tran <[email protected]>
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.
Pull Request Overview
This PR updates the payment channel example to work with go-perun v0.13.0 and adjusts client and app code accordingly. Key changes include modifications in client receiver types and method names, updates of error handling and context usage in channel methods, and changes within CI workflows and documentation to support the new dependency version.
Reviewed Changes
Copilot reviewed 547 out of 547 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
archived/collateralized-channels/client/util.go | New utility functions for channel operations and state updates. |
archived/collateralized-channels/client/handle.go | Renamed receivers and updated proposal/update handling logic. |
archived/collateralized-channels/client/eth.go | Updated transactor retrieval and receipt confirmation. |
archived/collateralized-channels/client/client.go | Introduced major client implementation with channel proposal and update logic. |
archived/collateralized-channels/app/util.go and app/app.go | Adjusted app data casting and encoding to use value types instead of pointers. |
archived/collateralized-channels/README.md | Updated documentation for clarity on dependency compatibility. |
app-channel/contracts/perun-eth-contracts, README.md, and CI workflows | Updated submodule commit, added archived examples documentation, and refined CI configuration. |
Comments suppressed due to low confidence (2)
archived/collateralized-channels/client/client.go:161
- Returning an error message based on a nil 'err' when the channel is not found can lead to unclear error propagation. Replace it with a meaningful error creation (e.g. errors.New("channel not found")).
if !ok { return errors.WithMessage(err, "getting channel") }
archived/collateralized-channels/client/client.go:177
- Similar to the previous case, returning an error using a nil 'err' when the channel is not found in the Settle method makes the error unclear. Create and return a new error message instead.
if !ok { return errors.WithMessage(err, "getting channel") }
if err != nil { | ||
return errors.WithMessage(err, "creating peers") | ||
} |
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.
An 'if err != nil' check is present immediately after initializing the peers slice, but no error is produced in the preceding code. Remove this check or replace it with the intended error handling logic.
if err != nil { | |
return errors.WithMessage(err, "creating peers") | |
} |
Copilot uses AI. Check for mistakes.
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.
Pull Request Overview
Updates the collateralized channels example to use go-perun v0.13.0, adjusts client APIs, and refreshes documentation and CI accordingly
- Bumps go-perun dependency and adapts API changes in client and app code
- Updates archived example README and main README to reflect new organization
- Adjusts GitHub Actions workflows (CI and archived CI) and submodule for perun-eth-contracts
Reviewed Changes
Copilot reviewed 547 out of 547 changed files in this pull request and generated 6 comments.
Show a summary per file
File | Description |
---|---|
archived/collateralized-channels/client/util.go | Adapted to new perun API, adjusted error messages |
archived/collateralized-channels/client/handle.go | Refactored handlers to use updated channel types and locking |
archived/collateralized-channels/client/eth.go | Switched to perunClient transactor and backend fields |
archived/collateralized-channels/client/client.go | Integrated new perunClient, fixed channel setup and funding logic |
archived/collateralized-channels/app/util.go | Migrated app data to value type, updated balance helpers |
archived/collateralized-channels/app/app.go | Implemented Encode/Clone for CollateralAppData |
archived/collateralized-channels/README.md | Lists dependencies (needs version bump) |
app-channel/contracts/perun-eth-contracts | Updated submodule commit |
README.md | Added “Archived Examples” section |
.github/workflows/ci.yml | Bumped DFX version, shortened sleep timings |
.github/workflows/ci-archived.yml | Introduced workflow for archived examples |
Comments suppressed due to low confidence (2)
archived/collateralized-channels/client/eth.go:27
- Move the
err
check before settingtr.Context
. IfNewTransactor
returns an error,tr
will be nil andtr.Context = ctx
will panic.
tr.Context = ctx
archived/collateralized-channels/app/util.go:89
wallet.Address
does not have aCmp
method. Restore the originalp.Equal(walletAddr)
or use the correct comparison API.
if p.Cmp(walletAddr) == 0 {
if err != nil { | ||
return errors.WithMessage(err, "creating peers") | ||
} |
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.
This err
is never set in this block (peers creation cannot error), so the check is dead code. Remove the if err != nil
and its branch.
if err != nil { | |
return errors.WithMessage(err, "creating peers") | |
} |
Copilot uses AI. Check for mistakes.
|
||
ch, ok := c.channels[peer] | ||
if !ok { | ||
return errors.WithMessage(err, "getting 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.
When ch
is not found, err
is nil, so WithMessage(nil,…)
returns nil. Use errors.New("getting channel: not found")
or attach a new error instead.
return errors.WithMessage(err, "getting channel") | |
return errors.New("getting channel: not found") |
Copilot uses AI. Check for mistakes.
|
||
ch, ok := c.channels[peer] | ||
if !ok { | ||
return errors.WithMessage(err, "getting 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.
Same issue as above: err
is nil when channel lookup fails. Replace with a fresh error like errors.New("channel not found")
.
return errors.WithMessage(err, "getting channel") | |
return errors.New("channel not found") |
Copilot uses AI. Check for mistakes.
return nil, errors.New("channel not found") | ||
} | ||
b, err = c.assetHolder.Holdings(nil, calcChannelCollateralID(ch.ID(), c.Address())) | ||
return b, errors.WithMessage(err, "reading peer collateral") |
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.
This error wraps a channel collateral lookup. Update the message to "reading channel collateral" to avoid confusion.
return b, errors.WithMessage(err, "reading peer collateral") | |
return b, errors.WithMessage(err, "reading channel collateral") |
Copilot uses AI. Check for mistakes.
# Collateralized Channels (Archived ⚠️) | ||
|
||
## Dependencies | ||
This example requires the [ganache-cli](https://github.com/trufflesuite/ganache-cli) `v6.12.2` and [go-perun](https://github.com/hyperledger-labs/go-perun) `v0.6.0` . Newer versions of the dependencies might not be compatible. |
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.
The README still references go-perun v0.6.0, but the example now uses v0.13.0. Please update the version requirement accordingly.
This example requires the [ganache-cli](https://github.com/trufflesuite/ganache-cli) `v6.12.2` and [go-perun](https://github.com/hyperledger-labs/go-perun) `v0.6.0` . Newer versions of the dependencies might not be compatible. | |
This example requires the [ganache-cli](https://github.com/trufflesuite/ganache-cli) `v6.12.2` and [go-perun](https://github.com/hyperledger-labs/go-perun) `v0.13.0` . Newer versions of the dependencies might not be compatible. |
Copilot uses AI. Check for mistakes.
./startdeploy.sh | ||
sleep 30 | ||
sleep 5 |
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.
[nitpick] Reducing the sleep from 30s to 5s may cause flaky CI runs if deployment takes longer. Consider a health check or a longer timeout.
Copilot uses AI. Check for mistakes.
Description
This is part of multiple PRs to solve the issue #29, specifically solve the sub-issue #32. It updates the go-perun's dependency to
v0.13.0
Location
/payment-channel/
Features
v0.13.0