24/Service registry#28
Draft
tahpot wants to merge 23 commits into
Draft
Conversation
Remaining: node-test/getlogs_*.js
BytesLib.sol Removed unnecessary functions VeridaDIDRegistry.sol Update function modifiers from 'public' to 'external'
- Add comments in the body of functions - Add discoverService() function
BlockchainCrazy95
force-pushed
the
feature/24-service-registry
branch
from
6830a7a to
326e631
Compare
tahpot
commented
May 19, 2022
| bool isRegistered = registeredIds[identity].contains(serviceId); | ||
| require(!isRegistered, "Service has already registered!"); | ||
| registeredIds[identity].add(serviceId); | ||
| AccountInfo storage _account = accountInfos[identity]; |
Member
Author
There was a problem hiding this comment.
First check the account exists?
Contributor
|
@BlockchainCrazy95 The comments are not quite in good order, can you please format function comments in this way. /**
* @dev Returns x raised to the n-th power.
*
* @param {number} x The number to raise.
* @param {number} n The power, must be a natural number.
* @return {number} x raised to the n-th power.
*/
function pow(x, n) {
...
}Refer to this if you need some more understanding: https://javascript.info/comments |
|
|
||
| const tokenFactory = await ethers.getContractFactory("MockVDA"); | ||
| const tokenContract = await tokenFactory.deploy(); | ||
| await tokenContract.deployed(); |
Contributor
There was a problem hiding this comment.
Please add a condition that this doesn't deploy the same(VDA) in mainnet (ETH or MATIC)
Also, add the part that we can feed in the 'actual' VDA token address for mainnet launches.
There was a problem hiding this comment.
I'm not sure what you mean. Can you kindly explain more about it?
It's a sample token for test. I won't deploy the Verida Token.
Contributor
There was a problem hiding this comment.
Testnet and mainnet deployment.
Contributor
|
Have one test case failing @ITStar10
|
SlitherRun slither and it showed below issues. There were two high level severity issues and fixed.
|
MythrilNo issues were detected. |
Update public methods to external
Added comment in NatSpec Updated configuration : using .env
Please kindly explain about how the process of updating service will work. In case of decreasing the maxAccounts. |
Remaining: node-test/getlogs_*.js
BytesLib.sol Removed unnecessary functions VeridaDIDRegistry.sol Update function modifiers from 'public' to 'external'
Added comment in NatSpec Updated configuration : using .env
…n-contracts into feature/10-did-registry
- Add comments in the body of functions - Add discoverService() function
ITStar10
reviewed
Jun 9, 2022
| struct AccountInfo { | ||
| address identity; | ||
| uint256 creditAmount; | ||
| } |
ITStar10
reviewed
Jun 9, 2022
| /** | ||
| * did => AccountInfo | ||
| */ | ||
| mapping (address => AccountInfo) accountInfos; |
Collaborator
There was a problem hiding this comment.
No need to use AccountInfo struct.
ITStar10
reviewed
Jun 9, 2022
| uint256 maxAccounts, | ||
| uint256 pricePerAccount, | ||
| bytes calldata signature | ||
| ) public onlyVerifiedSignature(identity, signature) { |
Collaborator
There was a problem hiding this comment.
Need to check out status of service.
ITStar10
reviewed
Jun 9, 2022
| address identity, | ||
| bytes32 serviceId, | ||
| bytes calldata signature | ||
| ) external onlyVerifiedSignature(identity, signature) { |
Collaborator
There was a problem hiding this comment.
Need to check service status.
ITStar10
reviewed
Jun 9, 2022
ITStar10
reviewed
Jun 9, 2022
Collaborator
ITStar10
left a comment
ITStar10
left a comment
There was a problem hiding this comment.
- Need updates for credit. Or it can be resolved while implementing treasury.
- Need service status check in functions
ITStar10
reviewed
Jun 9, 2022
ITStar10
reviewed
Jun 9, 2022
ITStar10
reviewed
Jun 9, 2022
ITStar10
reviewed
Jun 9, 2022
ITStar10
reviewed
Jun 9, 2022
ITStar10
reviewed
Jun 9, 2022
Collaborator
ITStar10
left a comment
ITStar10
left a comment
There was a problem hiding this comment.
- Consolidate unnecessary functions.
- Remove unnecessary checks on token transfer.
Remaining: CheckServiceAvailability for user User disconnect problem And other stuffs
…chain-contracts into feature/24-service-registry
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #24 .
Initial MVP of service registry.