porter-finance / v1-core

⛰️ Smart contracts powering the Porter protocol.
https://porter.finance
GNU Affero General Public License v3.0
4 stars 3 forks source link

Emit event during bond factory construction #261

Closed Namaskar-1F64F closed 2 years ago

Namaskar-1F64F commented 2 years ago

The state variables isIssuerAllowListEnabled and isTokenAllowListEnabled emit events when they are toggled throughout the bond factories' existence. During contract deployment the values are initialized to true, but this change isn't reflected through the events IssuerAllowListEnabled and TokenAllowListEnabled which usually signal the change. I propose we add the events to the constructor as such.

/*
    When deployed, the deployer will be granted the DEFAULT_ADMIN_ROLE. This
    gives the ability the ability to call `grantRole` to grant access to
    the ISSUER_ROLE as well as the ability to toggle if the allow list
    is enabled or not at any time. The allow list events are emitted as the
    value of the lists "changed" to true during construction.
*/
constructor() {
    tokenImplementation = address(new Bond());
    _grantRole(DEFAULT_ADMIN_ROLE, _msgSender());
    emit IssuerAllowListEnabled(true);
    emit TokenAllowListEnabled(true);
}

Also, yes "the ability the ability" is a typo included in the current contract 🖌️

RusseII commented 2 years ago

What would the benefit of this be over inferring this data? We get the same result by reading the IssuerAllowListEnabled and the TokenAllowListEnabled and setting them to the current values on _grantRole handler, or setting default value. This pattern seems a bit weird to me. Last time I talked with peteris he seemed to prefer having minimal events and using contract calls to get any additionally needed data

Namaskar-1F64F commented 2 years ago

The graph data is hardcoding that default "true" value in because it's set on the contract. If we have two bond factories and one of the allow lists is set as false by default, the resulting state of the newly deployed bond factory would be incorrectly reflected in the graph.

RusseII commented 2 years ago

The graph data is hardcoding that default "true" value in because it's set on the contract. If we have two bond factories and one of the allow lists is set as false by default, the resulting state of the newly deployed bond factory would be incorrectly reflected in the graph.

That's true but issue could also be elevated by reading the value via the grantRole handler. I'd probably lean towards that.

RusseII commented 2 years ago

This could go either way. I'd like to not do this based on my thoughts + peteris + auditors. Please re-open if you feel strongly about adding this!