Incremental namespace #15
Conversation
brickpop
force-pushed
the
incremental-namespace
branch
from
ea99e12
to
02965d1
Compare
| // Full list for exhaustive retrieval | ||
| bytes[] validatorList; // Public key array | ||
| address[] oracleList; // Oracles allowed to submit processes to the Vochain and publish results on the process contract | ||
| // Bool mapping for fast checking |
There was a problem hiding this comment.
I personally do not like having:
- Array of validators
- Mapping of validators
- Array of oracles
- Mapping of oracles
Btw, I trust your criteria and given the time constrains this is fine for me. But I would like to see how big the improvement is for having the data duplication and method complexity increased justified.
There was a problem hiding this comment.
Ideally it would be a mapping, but the problem is than mappings cannot be iterated or retrieved all at once.
Initially it was an array, and it was good for add and remove operations. However, it was bad for isOracle and isValidator since a for loop was needed to search within the array, in addition to the keccak256 of each array item. These methods are used when setting the results on chain and triggering binding actions.
The goal of adding a (redundant) mapping was to have isOracle and isValidator with constant cost, instead of linear. It is marginally more expensive to add an oracle/validator, but this is a one time operation. And from then on, all reads become much cheaper.
| emit ChainRegistered(chainCount); | ||
| newChainId = chainCount; | ||
|
|
||
| chainCount = chainCount + 1; |
There was a problem hiding this comment.
There are some languages where ident++ is discouraged/disallowed, because it may have an ambiguous behavior when combined in complex instructions. I thought Solidity to be the case, although it seems that it is actually possible to use it like this.
There was a problem hiding this comment.
Yep, thought the same but I tested it and it is not that kind of complex operation, it is fine having ++ and +=1 and I think is cleaner.
| * Registers the (processes) contract calling this function into a new namespace, assigning it to the given chainId. | ||
| * Returns the new namespace ID that has been registered | ||
| */ | ||
| function register() public override returns (uint32 result) { |
There was a problem hiding this comment.
I still see this function a bit problematic. I was about to write some solutions that came up to my mind but maybe it is better to tackle afterwards. But .. what about having this function payable and a fee that increments as the same msg.sender registers namespaces?
There was a problem hiding this comment.
I'm not sure on how useful it can be to disincentivize upgrading the processes contract. Registering a processes instance has no implications for us and network fees disincentivize sending useless transactions with the gas fees.
The only issue would be that a spammer could bump the namespace numbers artificially, but he/she would be burning ether for no real benefit.
| .oracles[currentLength - 1]; | ||
| namespaces[namespace].oracles.pop(); | ||
| // The mapping starts at index 1 | ||
| namespaceCount = namespaceCount + 1; |
This pull requests adapts the smart contracts to be ready for future flexible parameterization needs.
Summary of changes:
Processesis now part of the newResultssmart contract (future ERC3k work)Namespacescontract is now simply a central registry forProcessescontracts to self registerNamespacescontract now is calledGenesis, and it acts as a central registry where we define Chains (genesis, validators, oracles)processsettings/status. Is it a global contract setting.lib/index.tsinto separate files for more maintainable exportsNow:
register()method on theNamespacescontract, and receives a uniquenamespaceIdas a resultProcessescontract is deployed, its address should be set on the (already existing)ResultscontractResultscontract, the contract will jump to theProcessescontract it has defined and will set the givenprocessIdas completed(Status.RESULTS)Main tasks