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

Add trustping protocol #17

Merged
merged 4 commits into from
Jan 29, 2020
Merged

Conversation

gnarula
Copy link
Contributor

@gnarula gnarula commented Jan 28, 2020

The other agents (in Python and .NET) use the trustping message to acknowledge a connection. This PR adds a handler for trustping messages.

Usually agents use a ping to establish connection

Signed-off-by: Gaurav Narula <[email protected]>
@gnarula gnarula requested a review from a team as a code owner January 28, 2020 14:05
default:
throw new Error('Invalid message type for handler');
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rather create a handler for each type as it is with other handlers. We can group handlers by a protocol in handlers folder. Maybe it's not the best design, but let's keep it consistent for now. Or is there any problem with that I don't see? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

uhm no problem as such but I figured it's easy to see all handlers for a particular protocol at one place rather than switching b/w files.

You're right that this breaks consistency with what we have at the moment. I'll split this into two different handlers. Maybe we can have another issue that refactors the handlers and groups them by protocol type? If so, could you create one please?

import { ConnectionState } from '../connections/domain/ConnectionState';

export class TrustPingService {
process_ping(inboundMessage: InboundMessage, connection: Connection) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There is perhaps no rule about camelCase method/function naming, my fault. Could you please rename it? And if you want, ESLint rules update would be appreciated :)

I understand there will be exceptions, mainly in relation to message attributes defined by spec, but I would recommend to keep it camelCase whenever possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

aye aye!

Also changed1 method name to follow camelCase

Signed-off-by: Gaurav Narula <[email protected]>
@jakubkoci jakubkoci merged commit 0f3e41f into openwallet-foundation:master Jan 29, 2020
berendsliedrecht pushed a commit to berendsliedrecht/credo-ts that referenced this pull request Mar 29, 2022
feat(core): w3cCredentialRecord and w3cCredentialRepository
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