Split SubnetActor #411
Split SubnetActor #411
Conversation
src/interfaces/ISubnetActor.sol
Outdated
| @@ -49,3 +48,53 @@ interface ISubnetActor { | |||
| /// The reword includes the fixed reward for a relayer defined in the contract and `amount` of fees from the cross-messages. | |||
| function distributeRewardToRelayers(uint256 height, uint256 amount, QuorumObjKind kind) external payable; | |||
| } | |||
|
|
|||
| interface ISubnetActorXX { | |||
There was a problem hiding this comment.
This seems duplicated, @dnkolegov? Is this a merge snafu?
| import {LibSubnetActor} from "../lib/LibSubnetActor.sol"; | ||
| import {Pausable} from "../lib/LibPausable.sol"; | ||
|
|
||
| contract SubnetActorCheckpointingFacet is SubnetActorModifiers, ReentrancyGuard, Pausable { |
There was a problem hiding this comment.
Should we just drop the SubnetActor prefix from the contract name, since the scope is already implied by the import path? I find it too verbose.
There was a problem hiding this comment.
Do you mean renaming to Checkpointingfacet ?
There was a problem hiding this comment.
Then we will have two contracts with the same name: router/Checkpointingfacet and subnet/CheckpointinfFacet
| import {LibStaking} from "../lib/LibStaking.sol"; | ||
| import {LibSubnetActor} from "../lib/LibSubnetActor.sol"; | ||
|
|
||
| contract SubnetActorRewardFacet is IRelayerRewardDistributor, SubnetActorModifiers, ReentrancyGuard, Pausable { |
There was a problem hiding this comment.
Could you explain the rationale for introducing a new interface here? (IRelayerRewardDistributor)
There was a problem hiding this comment.
It is the same interface actually. We did not need that bif ISubnetActor interface, we needed only one function. I added that one function to IRelayerRewardDistributor
| IDiamond.FacetCut({ | ||
| facetAddress: address(louper), | ||
| action: IDiamond.FacetCutAction.Add, | ||
| functionSelectors: gwLoupeSelectors | ||
| functionSelectors: saLouperSelectors |
There was a problem hiding this comment.
Are these selectors and the cutter selectors sa or gw dependent? They are general-purpose, right?
There was a problem hiding this comment.
That's correct, but that was incorrect either way due to gw prefix
src/lib/LibPausable.sol
Outdated
| @@ -86,9 +99,9 @@ abstract contract Pausable { | |||
| * | |||
| * - The contract must be paused. | |||
| */ | |||
| function _unpause() internal { | |||
| _requirePaused(); | |||
| function _unpause() internal { | |||
| PausableStorage storage s = pausableStorage(); | |||
There was a problem hiding this comment.
What difference does this make?
There was a problem hiding this comment.
Use the same style across the codebase: first, get the storage, then do something.
There was a problem hiding this comment.
IMO this general pattern is unsafe. You should only fetch storage immediately before using it. Otherwise you could be doing a dirty write if you call other functions in between that end up touching that storage. For example, what if _requirePaused() wrote to storage? (it doesn't in this case, but in the future it could). The previous was safer.
This PR :