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.

Sign up for GitHub

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

Jump to bottom

Clean up contracts #3

Merged
merged 30 commits into from
Nov 16, 2021
Merged

Clean up contracts #3

merged 30 commits into from
Nov 16, 2021

Conversation

adlerjohn
Copy link
Member

@adlerjohn adlerjohn commented Nov 8, 2021 •
edited
Loading

@adlerjohn adlerjohn self-assigned this Nov 8, 2021
liamsi pushed a commit that referenced this pull request Nov 13, 2021
@alexanderbez
chore: refactor CLI framework (#3)
50ae9d9
@adlerjohn adlerjohn marked this pull request as ready for review November 14, 2021 03:25
@adlerjohn adlerjohn requested review from liamsi and mattdf November 14, 2021 03:25
liamsi
liamsi reviewed Nov 14, 2021
View reviewed changes
Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

I don't understand why contracts were moved from contracts/ to contracts/contracts/ (seems unnecessary/confusing and introduces a diff that is not really a change).

Additionally left some nits/questions.

solidity/contracts/contracts/HashingTest.sol Outdated Show resolved Hide resolved
solidity/contracts/contracts/QuantumGravityBridge.sol Outdated Show resolved Hide resolved
solidity/contracts/contracts/QuantumGravityBridge.sol Outdated Show resolved Hide resolved
solidity/contracts/scripts/deploy.ts Outdated Show resolved Hide resolved
solidity/contracts/contracts/QuantumGravityBridge.sol Outdated Show resolved Hide resolved
solidity/contracts/contracts/QuantumGravityBridge.sol Outdated Show resolved Hide resolved
solidity/contracts/contracts/QuantumGravityBridge.sol Outdated Show resolved Hide resolved
solidity/contracts/contracts/TestERC20.sol Outdated Show resolved Hide resolved
solidity/contracts/contracts/QuantumGravityBridge.sol Outdated Show resolved Hide resolved
mattdf
mattdf reviewed Nov 15, 2021
View reviewed changes
solidity/contracts/contracts/HashingTest.sol Outdated
checkpoint = keccak256(abi.encode(checkpoint, _validators[i], _powers[i]));
}
}

Copy link

Choose a reason for hiding this comment

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

This function will return without error even if fed an empty validator set - is that intended?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, don't see why not.

@adlerjohn adlerjohn requested a review from liamsi November 15, 2021 18:29
@adlerjohn
Copy link
Member Author

Note for reviewers: tests are expected to fail catastrophically since the contract is changed but not the orchestrator.

@liamsi
Copy link
Member

liamsi commented Nov 15, 2021

Note for reviewers: tests are expected to fail catastrophically since the contract is changed but not the orchestrator.

Ah OK. If you want we can also merge this only after I submit a PR against this branch to fix the tests.

liamsi
liamsi approved these changes Nov 15, 2021
View reviewed changes
Copy link
Member

@liamsi liamsi left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Changes look good to me!
I feel a bit uncomfortable merging this with all the failing tests but I think it's OK to do so if we fix those in the following days.

@adlerjohn adlerjohn merged commit e2a253f into master Nov 16, 2021
@adlerjohn adlerjohn deleted the adlerjohn/contract_mods branch November 16, 2021 00:09
@liamsi
Copy link
Member

liamsi commented Nov 16, 2021 •
edited
Loading

BTW, surprisingly integration tests pass locally! NVM, wrong branch. but it is actually the build that is failing.

Failing actions are:
image
I think only @Wondertan currently adheres to this but maybe we should actually make this a coding convention for all repos and use the above linter for this as well: https://www.conventionalcommits.org/en/v1.0.0/

Other than that, the build doesn't work:
image

@liamsi liamsi mentioned this pull request Nov 16, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattdf mattdf mattdf left review comments

@liamsi liamsi liamsi approved these changes

Assignees

@adlerjohn adlerjohn

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@adlerjohn @liamsi @mattdf