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

Hive vote #28

Merged
merged 7 commits into from
Jun 29, 2024
Merged

Hive vote #28

merged 7 commits into from
Jun 29, 2024

Conversation

sisygoboom
Copy link
Collaborator

  • refactor some business logic into a new hive service
  • create tests and fixes for hive voting
  • deleted an empty auth guard
  • some endpoints still had /api in them despite caddy now adding that part

src/repositories/hive-chain/hive-chain.repository.ts Outdated Show resolved Hide resolved
src/services/api/api.controller.ts Outdated Show resolved Hide resolved
Comment on lines +26 to +35
const [accountType, account, network] = sub.split('/');

if (!accountTypes.includes(accountType as AccountType)) {
throw new Error(`Invalid account type: ${accountType}`);
}

if (!network.includes(network as Network)) {
throw new Error(`Invalid network: ${network}`);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this need username (account) validation as well? If not, I would assume that the above checks are just sanity checks.

This also brings up a bigger question. What do usernames look like for DID accounts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe they're just ceramic IDs. account is just any network dependant unique identifier in this case

Copy link
Contributor

Choose a reason for hiding this comment

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

What are ceramic IDs? Do you mean did:key:....s?

Also, what about the question about validation?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah they're just sanity checks, they allow us to return them types the type for a username is just string

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok cool. I don't think this should block this PR, but it would probably be better for us to have a better understand of what the account portion of the sub look like for each network type.

src/services/uploader/uploading.controller.ts Outdated Show resolved Hide resolved
src/services/uploader/uploading.controller.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@Geo25rey Geo25rey left a comment

Choose a reason for hiding this comment

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

LGTM

@sisygoboom sisygoboom merged commit 18737b7 into main Jun 29, 2024
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