Add possibility for flexible did method type id for state contract and onchain issuer #251
Conversation
Pull Request Test Coverage Report for Build 10162198597Details
💛 - Coveralls |
| * @dev getIdType | ||
| */ | ||
| function getIdType(uint256 id) internal pure returns (bytes2) { | ||
| return bytes2(PrimitiveTypeUtils.uint256ToBytes(PrimitiveTypeUtils.reverseUint256(id))); |
There was a problem hiding this comment.
will test with some assembly function, maybe it can be done more gas efficient
contracts/lib/IdentityLib.sol
Outdated
| ) external { | ||
| require(depth <= IDENTITY_MAX_SMT_DEPTH, "SMT depth is greater than max allowed depth"); | ||
| self.stateContract = IState(_stateContractAddr); | ||
| require(self.stateContract.isIdTypeSupported(idType), "id type doesn't exist"); |
There was a problem hiding this comment.
Should be id type is not supported, because State contract doesn't know if it exists on other blockchains.
contracts/state/State.sol
Outdated
| @@ -145,7 +145,9 @@ contract State is Ownable2StepUpgradeable, IState { | |||
| bytes calldata methodParams | |||
| ) public { | |||
| if (methodId == 1) { | |||
| uint256 calcId = GenesisUtils.calcIdFromEthAddress(getDefaultIdType(), msg.sender); | |||
| bytes2 idType = GenesisUtils.getIdType(id); | |||
| require(_stateData.isIdTypeSupported[idType], "id type is not supported"); | |||
There was a problem hiding this comment.
You need to check if idType is supported in transitState function too.
Also you can put this check here before if (methodId == 1) {, because we may add more methods, but the check would be universal for all of them.
There was a problem hiding this comment.
After moving this check before if (methodId == 1) - 59 tests is failing
Just double check should I fix this test or leave idTypeCheck inside if (methodId == 1)...?
There was a problem hiding this comment.
@Kolezhniuk that shouldn't happen. Probably tests use identities created with different idType, so you just need to add this idType to supported after the deployment.
contracts/state/State.sol
Outdated
| * @dev Set IdType setter | ||
| * @param idType id type | ||
| */ | ||
| function setIdType(bytes2 idType) public onlyOwner { |
There was a problem hiding this comment.
Should be renamed to let's say setIdTypeSupported with additional boolean param supported, so that we could disable support for some idType.
contracts/state/State.sol
Outdated
| bytes2 idType = GenesisUtils.getIdType(id); | ||
| require(_stateData.isIdTypeSupported[idType], "id type is not supported"); |
There was a problem hiding this comment.
Repeatable code. Better to put it into some modifier or public method.
It's interesting BTW whether the isIdTypeSupported method will be used by clients with bytes2 idType parameter in reality. Maybe clients in most cases would extract type id from identifier before making the contract call. If that is the case uint256 id can be used instead. What do you think @vmidyllic, @OBrezhniev ?
There was a problem hiding this comment.
default type is fine, because onchain issuer during creation can't pass identifier but should check that id type is supported
There was a problem hiding this comment.
Agree with Vlad. Agree on modifier.
There was a problem hiding this comment.
idType is used further in code, but I've added modifier also
There was a problem hiding this comment.
upd: moved this code to separate function
contracts/state/State.sol
F438
Outdated
| bytes2 idType = GenesisUtils.getIdType(id); | ||
| require(_stateData.isIdTypeSupported[idType], "id type is not supported"); |
There was a problem hiding this comment.
Use the modifier instead
There was a problem hiding this comment.
@AndriianChestnykh not sure that use modifier is reasonable here. In any case I need to calculate idType to use it below in the code. Why do I need calculate idType it twice?
There was a problem hiding this comment.
ah, sorry I missed that
contracts/state/State.sol
Outdated
| bytes2 idType = GenesisUtils.getIdType(id); | ||
| require(_stateData.isIdTypeSupported[idType], "id type is not supported"); |
There was a problem hiding this comment.
ah, sorry I missed that
contracts/state/State.sol
Outdated
| } | ||
|
|
||
| /** | ||
| * @dev Set supported IdType setter | ||
| * @param idType id type | ||
| */ | ||
| function setIdTypeSupported(bytes2 idType) public onlyOwner { | ||
| _stateData.isIdTypeSupported[idType] = true; | ||
| function setSupportedIdType(bytes2 idType) public onlyOwner { |
There was a problem hiding this comment.
I asked for possibility to disable it too with a second param to the function.
| @@ -170,7 +170,10 @@ contract CredentialAtomicQueryV3Validator is CredentialAtomicQueryValidatorBase | |||
| function _checkAuth(uint256 userID, address ethIdentityOwner) internal view { | |||
| require( | |||
| userID == | |||
| GenesisUtils.calcIdFromEthAddress(getState().getDefaultIdType(), ethIdentityOwner), | |||
| GenesisUtils.calcIdFromEthAddress( | |||
| getState().checkSupportedIdTypeForId(userID), | |||
There was a problem hiding this comment.
Better move getting of idType outside of the calcIdFromEthAddress.
contracts/state/State.sol
Outdated
| * @dev Check if the id type is supported and return the id type | ||
| * @param id Identity | ||
| */ | ||
| function checkSupportedIdTypeForId(uint256 id) public view returns (bytes2) { |
There was a problem hiding this comment.
Idk, this function looks confusing to me. It's not modifier, but it does some checks, but also returns idType...
@AndriianChestnykh what do you think?
There was a problem hiding this comment.
I agree, it's confusing. I would suggest use combination of getIdType and then isIdTypeSupported instead in any place (both from off-chain clients and inside smart contracts)
There was a problem hiding this comment.
Folks, now we returning back to the code duplication issue that was mentioned in previous comments.
There was a problem hiding this comment.
True, getIdTypeIfSupported(uint256 id) (which throws if not) is the best I can figure out now then.
contracts/state/State.sol
Outdated
| */ | ||
| function setDefaultIdType(bytes2 defaultIdType) external onlyOwner { |
There was a problem hiding this comment.
We still need this function (setDefaultIdType) and we'll call it on polygon mainnet & amoy.
No description provided.