-
Notifications
You must be signed in to change notification settings - Fork 5
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
Update Perun demos to go-perun 0.10.6 #17
Conversation
1797ba0
to
a67ed85
Compare
…lity to go-perun 0.10.6 * client: Modify go-perun methods * workflow: Set ganache to latest Signed-off-by: Ilja von Hoessle <ilja@perun.network>
a4c2b77
to
74b8493
Compare
.github/workflows/ci.yml
Outdated
run: | | ||
docker run --rm --name ganache --detach --publish 8545:8545 ${{ env.gananache-image }} --account $KEY_DEPLOYER,$BALANCE --account $KEY_ALICE,$BALANCE --account $KEY_BOB,$BALANCE --blockTime=5 | ||
docker run --rm --name ganache --detach --publish 8545:8545 trufflesuite/ganache:latest --account $KEY_DEPLOYER,$BALANCE --account $KEY_ALICE,$BALANCE --account $KEY_BOB,$BALANCE --blockTime=5 --gasPrice=0 |
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.
You probably want to continue using ${{ env.gananache-image }}
as defined above?
Why are you using --gasPrice=0
here?
Cheers!
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.
Hi,
the goal of this PR is to update each demo to go-perun v0.10.6 one after another without breaking any non-updated. Ideally, the versions of the other tools/repositories should be bumped as well. In this case, the payment-channel demo did not work with v6.12.2
, as defined in env
, so I have changed it to latest
. If I would have defined trufflesuite/ganache:latest
to be in env
, used by every Ethereum demo in this repository, it might have broken the other Ethereum demos in the CI, which we want to avoid.
Concerning the --gasPrice=0
setting, I simply follow the README of that demo. Whether or not this is reasonable, is out of scope of this PR.
Best regards
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.
Concerning the --gasPrice=0 setting, I simply follow the README of that demo. Whether or not this is reasonable, is out of scope of this PR.
I see, might have been an oversight in the review then. I would usually suggest to run without setting gas price to 0, especially in the CI, as it tests for a more realistic setting.
Btw: Tutorial also does not set gas price, which is probably review more carefully.
https://labs.hyperledger.org/perun-doc/go-perun/payment_tutorial/test.html
If you want to be consistent, I suggest you rather change the README.
Regarding the first point: I typically recommend to use a fixed version in the CI, because you will get stable behavior over time. I understand that you want to use different ganache versions (what was the reason again for why the new version doesn't work with the old ganache?), you could still create a variable for the new version as before.
Anyways, not my responsibility anymore :D
Just saw this and thought I added a few comments on this open source review... All the best!
…lity to go-perun 0.10.6 * client: Modify go-perun methods * workflow: Set ganache to latest Signed-off-by: Ilja von Hoessle <ilja@perun.network>
Signed-off-by: Minh Huy Tran <huy.tranminh2804@gmail.com>
Signed-off-by: Minh Huy Tran <huy.tranminh2804@gmail.com>
…an up Signed-off-by: Minh Huy Tran <huy.tranminh2804@gmail.com>
7f37f7f
to
040fd24
Compare
c27498f
to
6204a66
Compare
@@ -61,7 +61,7 @@ func deployContracts(nodeURL string, chainID uint64, privateKey string) (adj, ah | |||
panic(err) | |||
} | |||
|
|||
const gasLimit = 2200000 // Must be sufficient for deploying TicTacToe.sol. | |||
const gasLimit = 2200000 // Must be sufficient for deploying TicTacToeApp.sol. |
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 is needed because previous gasLimit is not sufficient to deploy app.
app-channel/contracts/generate.sh
Outdated
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.
Update the outdated bindings generation script to the newest version of solc and abigen.
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.
App need to be upated to solidity 0.8.0 to match the template used by the perun-eth-backend and perun-eth-contracts
Signed-off-by: Minh Huy Tran <huy.tranminh2804@gmail.com>
Signed-off-by: Minh Huy Tran <huy.tranminh2804@gmail.com>
Signed-off-by: Minh Huy Tran <huy.tranminh2804@gmail.com>
… update Signed-off-by: Minh Huy Tran <huy.tranminh2804@gmail.com>
…acts still need update Signed-off-by: Minh Huy Tran <huy.tranminh2804@gmail.com>
Signed-off-by: Minh Huy Tran <huy@perun.network>
WARNING: Payment Channel Dot example depends on perun-polkadot-backend and perun-polkadot-node. The following changes must be made before continuing:
Afterward, the dependency in Note: It is also recommended to update the |
f80a744
to
6d470b5
Compare
Signed-off-by: Minh Huy Tran <huy@perun.network>
6d470b5
to
e08ec2c
Compare
Signed-off-by: Minh Huy Tran <huy@perun.network>
…multiledger-channel demo
9a33eaf
to
317e16b
Compare
LGTM |
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
No description provided.