M05 #27
M05 #27
Conversation
src/SphereXEngine.sol
Outdated
| event TxStartedAtIrregularDepth(); | ||
| event ConfigureRules(bytes8 oldRules, bytes8 newRules); | ||
| event AddedAllowedSenders(address[] senders); | ||
| event AddedAllowedSendersOnchain(address sender); |
There was a problem hiding this comment.
either change to (address[] senders) or the name to AddedAllowedSenderOnchain
src/SphereXProtectedBase.sol
Outdated
|
|
||
| /** | ||
| * @title SphereX base Customer contract template | ||
| * @dev notice this is an abstract |
src/SphereXProtectedBase.sol
Outdated
| /** | ||
| * @dev used when the client uses a proxy - should be called by the inhereter initialization | ||
| */ | ||
| function __SphereXProtected_init(address admin, address operator ,address engine) internal virtual { |
There was a problem hiding this comment.
Why this is virtual?
There was a problem hiding this comment.
to let the client inherit and change the initialization process if he wishes
There was a problem hiding this comment.
why do we want him to have the option of chaining the protected init process?
There was a problem hiding this comment.
its not a matter of will but a matter of convention
src/SphereXEngine.sol
Outdated
| @@ -32,7 +32,11 @@ contract SphereXEngine is ISphereXEngine, AccessControlDefaultAdminRules { | |||
| bytes32 internal constant DEACTIVATED = bytes32(0); | |||
| uint64 internal constant RULES_1_AND_2_TOGETHER = 3; | |||
|
|
|||
| // the index of the addAllowedSenderOnChain in the call flow | |||
| int256 internal constant ADD_ALLOWED_SENDER_ONCHAIN_INDEX = 5000; | |||
There was a problem hiding this comment.
Not sure 5000 is a good value. Maybe consider something around MAX_INT256
There was a problem hiding this comment.
@Michaelr-spherex check the change does not brake your life
There was a problem hiding this comment.
It will not. I just want to know the final value to adjust to it
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| @@ -377,16 +377,17 @@ contract SphereXProtectedTest is Test, CFUtils { | |||
| SomeContract(someContract).someFunc(); | |||
| } | |||
|
|
|||
| function test_factoryEngineDisabledr() public { | |||
| function test_factoryEngineDisabled() public { | |||
| spherex_engine.grantRole(spherex_engine.SENDER_ADDER_ROLE(), address(costumer_contract)); | |||
There was a problem hiding this comment.
The operator should be able to grant this role, not the admin
src/SphereXEngine.sol
Outdated
| @@ -165,6 +165,10 @@ contract SphereXEngine is ISphereXEngine, AccessControlDefaultAdminRules { | |||
| emit RemovedAllowedPatterns(patterns); | |||
| } | |||
|
|
|||
| function grantSenderAdderRole(address newSenderAdder) external onlyOperator { | |||
| super._grantRole(SENDER_ADDER_ROLE, newSenderAdder); | |||
There was a problem hiding this comment.
Is super necessary? I think it's just when you have the same function name
There was a problem hiding this comment.
fixed, and fixed test
No description provided.