BlockMeta Smart Contract #7
Conversation
aregng
force-pushed
the
feature/blockmeta
branch
from
8ee5a97 to
779d2b9
Compare
aregng
changed the base branch from
feature/automation_registry
to
feature/erc20Supra
There was a problem hiding this comment.
Why do we need separate BlockBasedCounter since we already have Counter contract?
There was a problem hiding this comment.
BlockBasedCounter allow increment only if called by privileged address, but yes if we can extend Counter to provide increment interface guarded with privileged address, then we can remove BlockBasedCounter
There was a problem hiding this comment.
The increment function in Counter is already guarded by owner and only owner can call it. So, we can get rid of BlockBasedCounter.
There was a problem hiding this comment.
BlockBasedCounter allow increment only if called by privileged address, but yes if we can extend Counter to provide increment interface guarded with privileged address, then we can remove BlockBasedCounter
| import {Ownable2StepUpgradeable} from "../lib/openzeppelin-contracts-upgradeable/contracts/access/Ownable2StepUpgradeable.sol"; | ||
| import {UUPSUpgradeable} from "../lib/openzeppelin-contracts/contracts/proxy/utils/UUPSUpgradeable.sol"; |
There was a problem hiding this comment.
@udityadav-supraoracles blockmeta is supposed to be deployed at Genesis. When it says Ownable2StepUpgradeable does it mean 2 step transfer or two step upgrade? We don't want two step transfer for blockmeta. What is 2 step upgrade, if there is such a thing?
There was a problem hiding this comment.
Ownable2StepUpgradeable refers to transfer of ownership in 2 steps.
In the current implementation whoever deploys the BlockMeta becomes the owner, if it's deployed using a MultiSig, the MultiSig will become the owner.
I'll change it to one step transfer.
The suffix "Upgradeable" in Ownable2StepUpgradeable means this contract is for upgradable contracts.
| address constant VM_SIGNER = address(0x5355500000000000000000000000000000000000); | ||
|
|
||
| struct Entry { | ||
| bytes4[] selectors; |
There was a problem hiding this comment.
@udityadav-supraoracles , what is a selector?
There was a problem hiding this comment.
The first 4 bytes of keccak256 of function signature is its selector. The calldata also has same first 4 bytes which is equal to selector.
| // prevent duplicate target | ||
| if (!e.exists) { | ||
| e.exists = true; | ||
| targets.push(target); | ||
| } |
There was a problem hiding this comment.
is enumerable set a better choice here than this mechanism? @udityadav-supraoracles
There was a problem hiding this comment.
Yes, considering we need a way to deregister, enumerableset would be better.
| if (!ok) { | ||
| emit CallFailed(target, selector, ret); | ||
| } else { | ||
| emit CallSucceeded(target, selector); | ||
| } |
There was a problem hiding this comment.
does this revert the whole tx or only the side effect by callee will not happen?
There was a problem hiding this comment.
It won't revert the transaction, it will only emit for the failed transactions and continue with execution.
-removed BlockBasedCounter -updated BlockMeta with deregister and enumerableSet -updated testcases
| function initialize(address _privileged) public initializer { | ||
| privilegedAddress = _privileged; |
There was a problem hiding this comment.
Counter is utilized in scope of Multisig tests as well, shouldn't they be also updated as the initialize function is updated
There was a problem hiding this comment.
Yes, I've updated tests for multisig.
| if (!msg.sender.isVmSigner()) revert CallerNotVmSigner(); // Caller must be VM Signer | ||
|
|
||
| uint256 tLen = registeredTargets.length(); | ||
| for (uint256 i; i < tLen; i++) { |
There was a problem hiding this comment.
I think it might be required to have an ability to define the order of the execution by target. But lets keep this question open and have it discussed with Dr. @sjoshisupra when he is back, an not block PR from finalization.
I will add a reference of this comment to the ticket so we do not forget.
This PR includes the BlockMeta smart contract. Related to Entropy-Foundation/smr-moonshot#2524