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(sdk): User id as HexString #2774

Merged
merged 70 commits into from
Nov 1, 2024
Merged

feat(sdk): User id as HexString #2774

merged 70 commits into from
Nov 1, 2024

Conversation

teogeb
Copy link
Contributor

@teogeb teogeb commented Oct 1, 2024

Summary

User id items are stored as arbitrary length string instead of EthereumAddress.

Renamed getAddess() in StreamrClient to getUserId(). This is a breaking change.

This is a preparation PR for #2780. This PR doesn't doesn't contain any changes to contract / The Graph interaction and therefore any user ids which are not exactly 20 bytes long don't work yet. (We should merge this PR to main just before that PR, or implement the contract interactions in a PR which has this PR as a base branch.)

Changes

The UserID helper type if now a a separate BrandedString. Additionally there is a new helper type UserIDRaw, and helper functions toUserId and toUserIdRaw. There is also the HexString helper type which is an alias to string.

In the API all user id inputs and outputs are HexString values. This is not a breaking change as earlier the inputs and outputs were either string or EthereumAddress values. (The EthereumAddress is a branded string, and it was used only in outputs, i.e. in the API all of these were just string values.)

In Message:

  • field type of publisherId

In StreamrClient:

  • return type of getUserId(),
  • field type of UserPermissionQuery#user
    • it is parameter type in hasPermission()
  • field type of UserPermissionAssignment#user
    • it is parameter/return type in getPermissions(), grantPermissions(), revokePermissions() and setPermissions()
  • return type of getStreamPublishers() and getStreamSubscribers()
  • parametrer type of isStreamPublisher(), isStreamSubscriber()
  • field type of SearchStreamsPermissionFilter#user
    • it is a parameter type in searchStreams()
  • changed type of ResendFromOptions#publisherId and ResendRangeOptions#publisherId
    • these are parameter types in resend()
  • parameter type of addEncryptionKey()

In Stream:

  • corresponding changes in hasPermission(), getPermissions(), grantPermissions(), revokePermissions() and setPermissions()

Internal

In StreamMessage the publisherId field type is UserID (i.e. the branded string) as we don't consider that class to be a part part of the public API. Also the related user id types in MessageID/GroupKeyRequest/GroupKeyResponse are UserID strings.

Future improvements

  • Change all EthereumAddress outputs to HexString outputs (e.g. getStorageNodes()) to unify the API
  • Could rename user field of UserPermissionAssignment#user and SearchStreamsPermissionFilter#user to userId to unify the API (we have typically the ID suffix in these, e.g. streamId and publisherId)
  • Could refactor some test so that we use randomUserId instead of fastWallet / ether's Wallet

@github-actions github-actions bot added network Related to Network Package test-utils Related to Test Utils Package cli-tools Related to CLI Tools Package utils sdk node labels Oct 1, 2024
@teogeb teogeb force-pushed the user-id-as-UInt8Array branch from 7da176e to ef6f4f1 Compare October 1, 2024 12:18
@teogeb teogeb force-pushed the user-id-as-UInt8Array branch from b64f7a3 to 9a5ad2c Compare October 1, 2024 13:24
@teogeb teogeb marked this pull request as ready for review October 1, 2024 16:08
@teogeb teogeb requested a review from harbu October 1, 2024 16:08
CHANGELOG.md Outdated Show resolved Hide resolved
packages/sdk/src/StreamIDBuilder.ts Show resolved Hide resolved
packages/sdk/src/StreamrClient.ts Outdated Show resolved Hide resolved
packages/sdk/src/encryption/PublisherKeyExchange.ts Outdated Show resolved Hide resolved
@teogeb teogeb requested a review from harbu October 18, 2024 13:17
@teogeb teogeb requested a review from jtakalai October 28, 2024 09:42
Copy link
Contributor

@jtakalai jtakalai left a comment

Choose a reason for hiding this comment

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

I'm not going to demand changes in this repo, but I don't really see what the point of breaking/removing the old function is, especially in cases where (currently) we only support authentication only with Ethereum keys (that will have a corresponding address). It doesn't feel like too much of a concession to leave the existing getAddress etc. methods that return the ID (if it's an address) or throw (if it's not). This wouldn't require so many changes from users, esp those who expect to be using Ethereum addresses only.

packages/sdk/src/Authentication.ts Show resolved Hide resolved
packages/sdk/src/StreamIDBuilder.ts Show resolved Hide resolved
@teogeb teogeb changed the title feat(sdk)!: User id as HexString feat(sdk): User id as HexString Nov 1, 2024
@teogeb
Copy link
Contributor Author

teogeb commented Nov 1, 2024

I'm not going to demand changes in this repo, but I don't really see what the point of breaking/removing the old function is, especially in cases where (currently) we only support authentication only with Ethereum keys (that will have a corresponding address). It doesn't feel like too much of a concession to leave the existing getAddress etc. methods that return the ID (if it's an address) or throw (if it's not). This wouldn't require so many changes from users, esp those who expect to be using Ethereum addresses only.

Re-added the getAddress() as you suggested. We could maybe remove/deprecate it later if we want to simplify the API (o need to have have two different names for the same thing). But as there are no other authentication methods yet, it makes sense to keep the alias for backwards compatibility.

The getUserId() is the preferred method to access user's id so that we'll have this kind of straighforward API:

await stream.grantPermissions({
    permissions: [StreamPermission.PUBLISH],
    userId: await publisher.getUserId()
})

@teogeb teogeb merged commit 85a9e03 into main Nov 1, 2024
24 checks passed
@teogeb teogeb deleted the user-id-as-UInt8Array branch November 1, 2024 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli-tools Related to CLI Tools Package docs network Related to Network Package node sdk test-utils Related to Test Utils Package utils
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants