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

feat(golem-network): added getIdentity method to GolemNetwork object #1073

Open
wants to merge 3 commits into
base: beta
Choose a base branch
from

Conversation

mgordel
Copy link
Contributor

@mgordel mgordel commented Sep 5, 2024

No description provided.

@@ -188,6 +189,12 @@ export type GolemServices = {
storageProvider: StorageProvider;
};

export type Identity = {
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't blindly forward the data without any documentation this way. It's not clear for the user of this API what glm.getIdentity().identity means as it's identity->identity and there's not tsdoc that explains particular fields to the user.

I've tested the code and get the following result from the getIdentity():

{
  identity: '0x46b1...',
  name: 'eth-warsaw-workshop-test',
  role: 'manager'
}

This is roughly similar to output of yagna app-key show eth-warsaw-workshop-test --json:

{
  "allowOrigins": [],
  "createdDate": "2024-08-30T08:29:07.361270875",
  "identity": "0x46b1...",
  "key": "ec...e3",
  "name": "eth-warsaw-workshop-test",
  "role": "manager"
}

I guess that the naming of things is broken on yagna's API and in fact we have a mixture of identity and app-key concepts, where an app-key belongs to an identity.

As far as I'm aware, "identity" of the "Golem User" is established by his ETH address. This is reflected by yagna id show:

{
  "Ok": {
    "alias": null,
    "deleted": false,
    "isDefault": true,
    "isLocked": false,
    "nodeId": "0x46b1..."
  }
}

So actually, I'd rather expect from glm.getIdentity(): string -> "0x46b1..." instead of the object which melts together the identity and app-key information.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's a bit confusing. I wanted to be consistent with yagna api. And the entity should probably contain fields: {nodeId, name, role} -> https://github.com/golemfactory/ya-client/blob/master/model/src/identity.rs

but for our purposes I agree that a single nodeId string might be better

@mgordel mgordel requested a review from grisha87 September 6, 2024 09:07
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.

2 participants