ValidatorProxy contract 🔀 (& Hardhat support for v0.8 contracts 👷‍♂️👀)

[alexroan]

No description provided.

[shortcut-integration]

This pull request has been linked to Clubhouse Story #7923: Create a ValidatorProxy contract.

[github-actions]

I see that you haven't updated any CHANGELOG files. Would it make sense to do so?

Resolved

[kaleofduty]

more feedback

[kaleofduty]

You could make this AggregatorValidatorInterface target to save yourself some casts

[alexroan]

This isn't ideal because this struct is also used for the aggregator, which doesn't conform to that interface.

Since this is the case, I don't like the idea of using addresses for some state variables and AggregatorValidatorInterface for others.

[kaleofduty]

How about separate structs for separate purposes?

[alexroan]

We could, but they are essentially the same thing, and it would make other state vars mismatch. So this:

  /// @notice Uses a single storage slot to store the current address
  struct ProxyConfiguration {
    address target;
    bool hasNewProposal;
  }

  // Configuration for the current aggregator
  ProxyConfiguration private s_currentAggregator;
  // Proposed aggregator address
  address private s_proposedAggregator;

  // Configuration for the current validator
  ProxyConfiguration private s_currentValidator;
  // Proposed validator address
  address private s_proposedValidator;

  event AggregatorProposed(
    address indexed aggregator
  );
  event AggregatorUpgraded(
    address indexed previous,
    address indexed current
  );
  event ValidatorProposed(
    address indexed validator
  );
  event ValidatorUpgraded(
    address indexed previous,
    address indexed current
  );
  /// @notice The proposed aggregator called validate, but the call was not passed on to any validators
  event ProposedAggregatorValidateCall(
    address indexed proposed,
    uint256 previousRoundId,
    int256 previousAnswer,
    uint256 currentRoundId,
    int256 currentAnswer
  );

becomes a bit messier:

  /// @notice Uses a single storage slot to store the current address
  struct AggregatorConfiguration {
    address target;
    bool hasNewProposal;
  }

  struct ValidatorConfiguration {
    AggregatorValidatorInterface target;
    bool hasNewProposal;
  }

  // Configuration for the current aggregator
  AggregatorConfiguration private s_currentAggregator;
  // Proposed aggregator address
  address private s_proposedAggregator;

  // Configuration for the current validator
  ValidatorConfiguration private s_currentValidator;
  // Proposed validator address
  AggregatorValidatorInterface private s_proposedValidator;

  event AggregatorProposed(
    address indexed aggregator
  );
  event AggregatorUpgraded(
    address indexed previous,
    address indexed current
  );
  event ValidatorProposed(
    AggregatorValidatorInterface indexed validator
  );
  event ValidatorUpgraded(
    AggregatorValidatorInterface indexed previous,
    AggregatorValidatorInterface indexed current
  );
  /// @notice The proposed aggregator called validate, but the call was not passed on to any validators
  event ProposedAggregatorValidateCall(
    address indexed proposed,
    uint256 previousRoundId,
    int256 previousAnswer,
    uint256 currentRoundId,
    int256 currentAnswer
  );

Since aggregators will be the address type anyway, it starts to get a bit messy. It's purely aesthetic, but I feel using address across the board and having a single struct is more elegant.

[kaleofduty]

To me the second looks cleaner plus you get more type safety.

[kaleofduty]

Ditto, could be AggregatorValidatorInterface

[kaleofduty]

ditto

[kaleofduty]

Suggested change

	require(current.hasNewProposal == true, "No proposal");
	require(current.hasNewProposal, "No proposal");

[kaleofduty]

I would prefer initializing a new struct here since it forces you to not forget any fields

Suggested change

	s_currentAggregator = current;
	s_currentAggregator = ProxyConfiguration({
	target: proposed,
	hasNewProposal: false
	});

[kaleofduty]

ditto struct initializer

[kaleofduty]

we usually try to filter duplicate logs

[kaleofduty]

ditto duplicate logs

[kaleofduty]

Could you initialize these using the full struct initializer syntax as well?