Conversation
xklob
left a comment
xklob
left a comment
There was a problem hiding this comment.
See comments; also the tests here are only testing view functions. I'd like to see a few tests with this applied to user flows within the system (so I guess integration tests) before approving.
|
@Joeysantoro Have we run integration tests on this? |
There was a problem hiding this comment.
See comments.
Honestly the way that this revokes the roles breaks some, imo, very important patterns and makes me very wary as is:
-
any contract that might exist that calls fei.getCore() to do some operation via the core will break. even if we don't have any contract that does this (I'm not sure if we don't), there's probably other contracts by external parties that do. one solution to this is to proxy-through all calls that aren't the ones we define in restrictedPermissions, which is not hard to do, and I think probably a much better pattern
-
I really dislike reverting when calling a view/pure function, especially one that should be a getter. "isGovernor" should never revert, just return false if disabled as in this case.
| }); | ||
|
|
||
| describe('Reserve Stabilizer', async () => { | ||
| describe.skip('Reserve Stabilizer', async () => { |
|
also should fix e2e tests or skip them if irrelevant |
test/integration/tests/fei.ts
Outdated
| await resetFork(); | ||
| }); | ||
|
|
||
| describe.only('e2e-fei', function () { |
|
Looks good to me - just fix that .only and should be fine. The failing e2e tests are annoying so if you could fix them too that'd be great, but they aren't relevant to this PR. |
3 participants
Adds a RestrictedPermissions contract which ignores or reverts on checks for burner, governor, and pcv controller.
It passes through checks for minter and guardian to the main core contract.
The intended usage is to setCore() on Fei contract so the burner functionality can no longer be used, nor transfer hooks.