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

SMT tests & linting #20

Merged
merged 9 commits into from
Jun 7, 2018
Merged

SMT tests & linting #20

merged 9 commits into from
Jun 7, 2018

Conversation

aupiff
Copy link
Contributor

@aupiff aupiff commented Jun 7, 2018

All the interesting changes happened in python code, ignore most of the solidity stuff because it's just linting.

  • flake8 linting
  • solium linting (added lint check to Jenkins)
  • expanded tests for SMT, cleaned up SMT Python code
  • improved interface for challengeBefore in python client

thoughts:

  • shouldn't always have 8 bytes of proof data in SMT impl if we allow depth to be dynamic
  • allowing depth to be dynamic means that we have to check that all uids used in tree are less than 2**(depth-1)
  • why are rootchain events in a separate contract? I'd prefer to put them in the main RootChain contract (object).

@aupiff aupiff added the WIP label Jun 7, 2018
@aupiff aupiff changed the title Challenge tests, SMT tests & linting SMT tests & linting Jun 7, 2018
@aupiff aupiff removed the WIP label Jun 7, 2018
@gakonst
Copy link
Contributor

gakonst commented Jun 7, 2018

why are rootchain events in a separate contract? I'd prefer to put them in the main RootChain contract (object).

No objection.

shouldn't always have 8 bytes of proof data in SMT impl if we allow depth to be dynamic

True, if depth is dynamic, the number of bytes should always be $depth/8$.

allowing depth to be dynamic means that we have to check that all uids used in tree are less than 2**(depth-1)

Also true.

Although allowing the tree to have dynamic depth is a good upgrade, I believe that we can leave that for later.

block = self.get_block(exiting_tx_block_num)
exiting_tx = block.get_tx_by_uid(uid)
# make sure this is inclusion
exiting_tx_inclusion_proof = self.get_proof(exiting_tx_block_num, uid)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is exactly what I had in mind, great

bytes prevTxBytes, bytes exitingTxBytes,
bytes prevTxInclusionProof, bytes exitingTxInclusionProof,
bytes sig,
uint64 slot, bytes prevTxBytes,
Copy link
Contributor

Choose a reason for hiding this comment

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

The reason I had the arguments formatted like that is because I find them easier to read. Left side is the previousTx information, right side the exiting tx.

@gakonst gakonst merged commit 5aa7705 into master Jun 7, 2018
@gakonst gakonst mentioned this pull request Jun 7, 2018
@aupiff aupiff deleted the challenges branch June 7, 2018 15:11
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.

None yet

2 participants