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

Use validator set precompiles in Attestations #1248

Merged
merged 10 commits into from
Oct 22, 2019
Merged

Use validator set precompiles in Attestations #1248

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
5c204df
Use validator set precompiles in Attestations
m-chrzan Oct 2, 2019
ee13557
Merge branch 'master' into m-chrzan/use-validator-precompile
m-chrzan Oct 14, 2019
d3da2f0
Use precompiles in Attestations directly
m-chrzan Oct 16, 2019
b8e9211
Merge branch 'master' into m-chrzan/use-validator-precompile
m-chrzan Oct 16, 2019
fb9aba1
Document use of test contract
m-chrzan Oct 16, 2019
8701010
Merge branch 'master' into m-chrzan/use-validator-precompile
m-chrzan Oct 16, 2019
ec08cdd
Move precompile usage to separate contract
m-chrzan Oct 16, 2019
535b8e1
Remove superflous methods
nambrot Oct 17, 2019
ce76b44
Merge branch 'master' into m-chrzan/use-validator-precompile
nambrot Oct 17, 2019
31905d2
Merge branch 'master' into m-chrzan/use-validator-precompile
cmcewen Oct 22, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/protocol/contracts/governance/interfaces/IValidators.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,6 @@ interface IValidators {
function isVoting(address) external view returns (bool);
function isValidating(address) external view returns (bool);
function getValidators() external view returns (address[] memory);
function validatorAddressFromCurrentSet(uint256) external view returns (address);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this in the IValidators interface?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Leftover from before moving the precompile into Attestations.

Copy link
Contributor

Choose a reason for hiding this comment

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

So remove it?

function numberValidatorsInCurrentSet() external view returns (uint256);
}
8 changes: 8 additions & 0 deletions packages/protocol/contracts/governance/test/MockValidators.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,14 @@ contract MockValidators is IValidators {
return validators;
}

function numberValidatorsInCurrentSet() external view returns (uint256) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this in MockValidators?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as the IValidators issue below.

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove it?

return validators.length;
}

function validatorAddressFromCurrentSet(uint256 index) external view returns (address) {
return validators[index];
}

function setValidating(address account) external {
_isValidating[account] = true;
}
Expand Down
9 changes: 7 additions & 2 deletions packages/protocol/contracts/identity/Attestations.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -642,16 +642,21 @@ contract Attestations is IAttestations, Ownable, Initializable, UsingRegistry, R
internal
{
IRandom random = IRandom(registry.getAddressForOrDie(RANDOM_REGISTRY_ID));
IValidators validators = IValidators(
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to call into the Validators smart contract for this, or can this contract call the precompile directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could. I was imagining having Validators expose a wrapper around the precompile to minimize complexity (with assembly in other contracts), though this does increase gas costs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree with wanting to cut gas costs, but there is also a small technical problem I just ran into, that the current solution kind of nicely circumvents.

If we use the precompile directly, we can't do ganache tests if we don't modify ethereumjs-vm to handle this precompile (which is not trivial since ganache has no concept of a validator set). By calling into the precompile wrapper in Validators, we can mock Validators and still test this function.

Copy link
Contributor

Choose a reason for hiding this comment

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

As we discussed offline, I think it's probably best to call the precompile directly in a privaet function, and to have a TestAttestations contract that inherits from Attestations and mocks out the call to the precompile

Copy link
Contributor

Choose a reason for hiding this comment

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

Consider defining a UsingPrecompiles contract, e.g. https://github.com/celo-org/celo-monorepo/pull/1292/files#diff-0cffec14a25e6876b387a826c4b4946d

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to add some support for this to ganache. If not, we can't call any part of core contracts that use validators.

Maybe define a list of addresses to serve as validators when launching it

registry.getAddressForOrDie(VALIDATORS_REGISTRY_ID)
);

bytes32 seed = random.random();
address[] memory validators = getValidators();
uint256 numberValidators = validators.numberValidatorsInCurrentSet();

uint256 currentIndex = 0;
address validator;

while (currentIndex < n) {
seed = keccak256(abi.encodePacked(seed));
validator = validators[uint256(seed) % validators.length];
validator = validators.validatorAddressFromCurrentSet(
m-chrzan marked this conversation as resolved.
Show resolved Hide resolved
uint256(seed) % numberValidators
);

Attestation storage attestations =
state.issuedAttestations[validator];
Expand Down