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 periods check on proof in challengeExit() #270

Merged
merged 4 commits into from
Feb 3, 2020

Conversation

PumpkingWok
Copy link
Contributor

@PumpkingWok PumpkingWok commented Jan 11, 2020

Description:

  • Add check on proofs inclusion into the Bridge (both prevProof and proof)
  • Add test

Fixes: #260

Hi there,

I have added the same require from startExit() function regard proof, i added the check for both proofs. I hope to understood properly. Correct me if i'm wrong.
Thanks in advance.

@johannbarbie
Copy link
Member

looks great. to get around the gas limit problem for truffle i did this to truffle-config.js:

-          runs: 200,
+          runs: 100,

@PumpkingWok
Copy link
Contributor Author

Hi @johannbarbie,

Thanks a lot for the info, but i'm still encountering this issue.
I tried both yarn test and yarn test ganache but they fail on FastExitHandler during the test cause out of gas.

Also i tried to add gas and gasPrice in truffle config for development, ganache network and directly in solc compiler but nothing.

Thank in advance

@johannbarbie
Copy link
Member

but they fail on FastExitHandler during the test cause out of gas.

the reason being, that the contract needs more gas than blockgaslimit to be deployed. the only way around that is to find a way to remove some code at another place.

@PumpkingWok
Copy link
Contributor Author

Hi @johannbarbie,
Thank you for the reply, i moved the unpackSignedData from FastExitHandler to a new contract, now it is work.

@simonweniger
Copy link
Member

Hey @PumpkingWok ,
how is it going ? Do you need any help ?

@PumpkingWok
Copy link
Contributor Author

Hi @simonweniger,
I have just created a new commit, i added a new test in exitHandler.js, it does not allow to challengeExit from a deleted period.
I tried all use cases locally (delete both periods, only one, ecc) and it works but in the code i delete only _proof period. I can edit the code without problems, let me know.
Thanks in advance.

@johannbarbie
Copy link
Member

@PumpkingWok
i don't really understand your question. is the PR ready for review, or are you asking for help? if former, plz remove the [WIP] from PR name :)

@PumpkingWok
Copy link
Contributor Author

PumpkingWok commented Jan 28, 2020

Hi @johannbarbie ,
Yes the PR is ready for review, i did not ask any question, let me know.
Thanks

@PumpkingWok PumpkingWok changed the title [WIP] add periods check on proof in challengeExit() Add periods check on proof in challengeExit() Jan 28, 2020
@johannbarbie
Copy link
Member

@PumpkingWok i'm receiving the following error when trying to run yarn test on your branch:

Error: Error: Error: while migrating FastExitHandler: Returned error: VM Exception while processing transaction: out of gas
    at Object.run (/Users/johba/dev/leap-contracts/node_modules/truffle/build/webpack:/packages/truffle-migrate/index.js:92:1)
    at processTicksAndRejections (internal/process/task_queues.js:86:5)

Copy link
Member

@johannbarbie johannbarbie left a comment

Choose a reason for hiding this comment

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

i'm receiving the following error when trying to run yarn test on your branch:

Error: Error: Error: while migrating FastExitHandler: Returned error: VM Exception while processing transaction: out of gas
    at Object.run (/Users/johba/dev/leap-contracts/node_modules/truffle/build/webpack:/packages/truffle-migrate/index.js:92:1)
    at processTicksAndRejections (internal/process/task_queues.js:86:5)

@johannbarbie
Copy link
Member

@PumpkingWok i have an easy proposal to resolve this:

  1. change the following line:
diff --git a/truffle-config.js b/truffle-config.js
index be3ba91..636321e 100644
--- a/truffle-config.js
+++ b/truffle-config.js
@@ -17,7 +17,7 @@ module.exports = {
       settings: {
         optimizer: {
           enabled: true,
-          runs: 200,
+          runs: 2,
         },
  1. open an issue in this repo to fix contract size problem.

then i can approve the pr.

@PumpkingWok
Copy link
Contributor Author

Hi @johannbarbie,
Did you resolve changing the runs parameter ? i don't locally, i can resolve only moving unpackedSignedData in another new contract.
If you want i can edit runs parameter as you told me and create a new commit in this PR, also i can open a new issue in this repo for notify the problem regard FaxExitHandler gas limit.
Thanks in advance

@johannbarbie
Copy link
Member

the runs parameter resolves it for me. maybe you have to rm -R build in between.
let's take the easiest way, up to you.

@PumpkingWok
Copy link
Contributor Author

oh i'm sorry, i have just tried to remove build and it works now using 2 in refs. I'm going to commit this edit and i can open an issue without problem if it is still necessary. let me know. thank.

@johannbarbie
Copy link
Member

@PumpkingWok of the two requests i had i see that you addressed 1., but i don't see 2. open an issue in this repo to fix contract size problem..

Copy link
Member

@johannbarbie johannbarbie left a comment

Choose a reason for hiding this comment

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

good work 👍

@johannbarbie johannbarbie merged commit eecdc8e into leapdao:master Feb 3, 2020
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.

challengeExit() needs to check that the periods the proofs reference were actually submitted to the bridge.
3 participants