fix: check before adding/removing whitelisted addresses [N-23]

[pcarranzav]

No description provided.

[codecov]

Codecov Report

Merging #624 (37d70a5) into pcv/571-n21-unused-imports (960cc04) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@                     Coverage Diff                     @@
##           pcv/571-n21-unused-imports     #624   +/-   ##
===========================================================
  Coverage                       91.91%   91.92%           
===========================================================
  Files                              44       44           
  Lines                            2078     2080    +2     
  Branches                          354      356    +2     
===========================================================
+ Hits                             1910     1912    +2     
  Misses                            168      168           

Flag	Coverage Δ
unittests	91.92% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
contracts/gateway/L1GraphTokenGateway.sol	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 960cc04...37d70a5. Read the comment docs.

[abarmat]

Just nitpicking a bit, this function might also have been something like:

function setCallhookWhitelist(address _target, bool _enabled) external onlyGovernor

But your version is more explicit, it's good 👍

In the spirit of failing loudly, I'd revert if the callhook is already whitelisted like:

require(callhookWhitelist[_newWhitelisted] == false, "WHITELIST_SET");

I try to avoid a transaction that is successful and doesn't do a state change.

What do you think?

[pcarranzav]

Just nitpicking a bit, this function might also have been something like:

function setCallhookWhitelist(address _target, bool _enabled) external onlyGovernor

But your version is more explicit, it's good 👍

Cool, yeah, I wasn't sure which way was better, I wonder if any of the options is a bit more gas-efficient, but in the end this won't be called very often so probably no big deal either way...

In the spirit of failing loudly, I'd revert if the callhook is already whitelisted like:

require(callhookWhitelist[_newWhitelisted] == false, "WHITELIST_SET");

I try to avoid a transaction that is successful and doesn't do a state change.

What do you think?

Good point, I wasn't sure what to do for this, since it's technically okay to try to set the whitelisted address twice, and I liked the function being idempotent, but in this case it might be safer to revert as it might be a sign that the user did something wrong...

[pcarranzav]

Amended to incorporate @abarmat's suggestion to revert rather than silently skip

The base branch was changed.