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.

Sign up for GitHub

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
Open

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

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
63b5b24
feat(golem-network): added getIdentity method to GolemNetwork object
mgordel Sep 5, 2024
2d1e029
chore: added own type Identity
mgordel Sep 5, 2024
8d4eb82
refactor: getIdentity returns only nodeId string
mgordel Sep 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion src/golem-network/golem-network.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ import { DataTransferProtocol } from "../shared/types";
import { NetworkApiAdapter } from "../shared/yagna/adapters/network-api-adapter";
import { IProposalRepository } from "../market/proposal";
import { Subscription } from "rxjs";
import { GolemUserError } from "../shared/error/golem-error";

/**
* Instance of an object or a factory function that you can call `new` on.
Expand Down Expand Up @@ -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

identity: string;
name: string;
role: string;
};

/**
* General purpose and high-level API for the Golem Network
*
Expand Down Expand Up @@ -226,6 +233,8 @@ export class GolemNetwork {
*/
private cleanupTasks: (() => Promise<void> | void)[] = [];

private identity?: Identity;

constructor(options: Partial<GolemNetworkOptions> = {}) {
const optDefaults: GolemNetworkOptions = {
dataTransferProtocol: isNode ? "gftp" : "ws",
Expand Down Expand Up @@ -321,7 +330,7 @@ export class GolemNetwork {
*/
async connect() {
try {
await this.yagna.connect();
this.identity = await this.yagna.connect();
await this.services.paymentApi.connect();
await this.storageProvider.init();
this.events.emit("connected");
Expand Down Expand Up @@ -632,6 +641,13 @@ export class GolemNetwork {
return this.hasConnection;
}

getIdentity(): Identity {
if (!this.isConnected() || !this.identity) {
throw new GolemUserError("To obtain an Identity, you must first connect to the Golem Network.");
}
return this.identity;
}

/**
* Creates a new logical network within the Golem VPN infrastructure.
* Allows communication between network nodes using standard network mechanisms,
Expand Down