Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversion of Name to Id #85

Merged
merged 6 commits into from
Dec 17, 2018
Merged

Conversion of Name to Id #85

merged 6 commits into from
Dec 17, 2018

Conversation

ksridharbabuus
Copy link
Contributor

Converting name index to Id index for better uniqueness of the Organizations, services and type repos. Added Organization Name and updated few methods to refer by Id instead of Name.

@ksridharbabuus
Copy link
Contributor Author

@astroseger You can also start the changes in the CLI based on the Registry Function prototype changes. Mostly the following changes are required:

  1. orgName -> orgId
  2. serviceName -> serviceId
  3. typeRepoName -> typeRepoId
  4. Method Name change for:
    getOrganizationByName to getOrganizationById
    getServiceRegistrationByName to getServiceRegistrationById
    getTypeRepositoryByName to getTypeRepositoryById
    Also in this methods the return variable name is also changed to id. Should not impact on the Client Code.
  5. Add orgName to createOrganization function call.

@astroseger
Copy link
Collaborator

astroseger commented Dec 8, 2018

@ksridharbabuus

  1. I'm strongly against including "build" folder to github. It is like uploading "exe" files. It is misusing of git. And it is completely unnecessary. We have github releases and npm releases for storing all these abi files (including network files). see Automate publishing development versions of the package #53 .

    • The algorithm is following - we deploy contracts to kovan (And to mainnent, when the time comes), and we make github release and npm release. We already did it for https://github.com/singnet/platform-contracts/releases/tag/v0.2.4 with @Dinesh2521. And it works fine. Both snet-cli and deaemon get abi and networks from releases (and I hope dApp too).
  2. Probably we should have function to change organization name (only owner should be able to do so)

@ksridharbabuus
Copy link
Contributor Author

@astroseger I dont like to keep Build folder in the git. It should be part of git ignore, mostly it might have missed. Will remove it. Thanks!

Will add the function to update orgName as we dont have any method to modify the same. Will do it. Thanks!

@astroseger
Copy link
Collaborator

@ksridharbabuus
OrgName should be string or at least bytes (not bytes32)

@ksridharbabuus
Copy link
Contributor Author

will do the change

@ksridharbabuus
Copy link
Contributor Author

@astroseger Changes to Registry are done.

@astroseger
Copy link
Collaborator

But why bytes? I've already change snet-cli to deal with orgName type=string....

@astroseger
Copy link
Collaborator

Actually it is not very important... But I think string makes more sense for orgName to avoid encoding and decoding to and from utf-8.

@ksridharbabuus
Copy link
Contributor Author

@astroseger I prefer to use bytes compared to string in Solidity. I agree there might be additional function call in f/w to decode the value. Let us keep it as bytes.

@ksridharbabuus
Copy link
Contributor Author

@astroseger In case if it takes longer time for the CLI changes (String to bytes) then I can convert to String in the Registry Contract. Let me know. Thanks!

@astroseger
Copy link
Collaborator

@ksridharbabuus disadvantage of keeping it in "bytes" is following: we have to agree in which encoding we store organizationName. And we need to consistently follow this agreement in all apps.

If we keep it in "string" then this "agreement" is already fixed in solidity, and we don't need to deal with it. So it is a little bit more clean. But of course difference is not so important. @vforvalerio87 @tiero What do you think?

(from other side I chose bytes for metadataURI, because in the future it might be something more complicated than simple utf-8 encoded string. For example it could be http URI + binary hash)

@ksridharbabuus
Copy link
Contributor Author

@astroseger I dont want restrict to UTF8 for the Orgname to enable other languages as well. If we are fine only to have UTF8 then I can convert to string.

@ksridharbabuus
Copy link
Contributor Author

@astroseger anyway for globalization we need to change in lots of other places. Let me change it string then.

@@ -1,5 +1,7 @@
"use strict";
var Registry = artifacts.require("./Registry.sol");
let { HELPERS } = require("./util/Util.js");
let { bytesToString} = HELPERS;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need bytesToString anymore.. But it is a minor comment anyway...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants