-
Notifications
You must be signed in to change notification settings - Fork 38
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
chore: Test the main contract #100
Conversation
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.
Thanks for adding the checkpoint test :D
@@ -1,3 +1,6 @@ | |||
[submodule "ethereum/solidity/lib/ds-test"] | |||
path = ethereum/solidity/lib/ds-test | |||
url = https://github.com/dapphub/ds-test | |||
[submodule "ethereum/solidity/lib/openzeppelin-contracts"] |
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.
I wonder how were we able to run tests before without having this set here :D
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.
Ah, so forge build
doesn't actually error if it can't find an import... forge test
does, but given that the QGB contract itself was not tested prior to this PR, then of course there would be no error.
This is of course one of the many examples to Solidity tooling being extremely immature. In the future we should examine using Sway and relaying directly to a Fuel execution layer.
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.
I'm not a solidity expert but this LGTM. Can we reject PRs without unit tests in the future? They should be part of any feature introduced.
function computeValidatorSetHash(Validator[] memory _validators) private pure returns (bytes32) { | ||
return keccak256(abi.encode(_validators)); | ||
} | ||
|
||
function domainSeparateValidatorSetHash( | ||
bytes32 _bridge_id, | ||
uint256 _nonce, | ||
uint256 _powerThreshold, | ||
bytes32 _validatorSetHash | ||
) private pure returns (bytes32) { | ||
bytes32 c = keccak256( | ||
abi.encode(_bridge_id, VALIDATOR_SET_HASH_DOMAIN_SEPARATOR, _nonce, _powerThreshold, _validatorSetHash) | ||
); | ||
|
||
return c; | ||
} |
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.
Rather than re-implementing these (which is hard to maintain), you could instead make a mock contract that wraps QGB and simply exposes internal methods and public, then call that.
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.
I tried, but couldn't get the mocks to work. per the docs
Private functions and state variables are only visible for the contract they are defined in and not in derived contracts.
does this mean that we can't use them in a mock contract? I could still be doing something wrong
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 won't compile
contract ExposedQuantumGravityBridge is QuantumGravityBridge {
function _computeValidatorSetHash(Validator[] memory _validators) public pure returns (bytes32) {
return computeValidatorSetHash(_validators);
}
function _domainSeparateValidatorSetHash(
bytes32 _bridge_id,
uint256 _nonce,
uint256 _powerThreshold,
bytes32 _validatorSetHash
) public pure returns (bytes32) {
return domainSeparateValidatorSetHash(_bridge_id, _nonce, _powerThreshold, _validatorSetHash);
}
}
Co-authored-by: John Adler <adlerjohn@users.noreply.github.com>
Co-authored-by: John Adler <adlerjohn@users.noreply.github.com>
Co-authored-by: John Adler <adlerjohn@users.noreply.github.com>
This PR starts the tests for the main contracts.
While this is a decent start, I'm not sure we can close #98 as this is still far from appropriate coverage percentage imho. I didn't originally set out to close #98, only do complete enough to help me debug the relayer. We still need to test expected failures, nil signatures, and a few other edge cases.