Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: middleware support for ICS20 #533
feat: middleware support for ICS20 #533
Changes from all commits
ae0629b
9b4ac36
a019faf
e18c29f
5826f83
13bb71e
4d8f264
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reasoning for now adding callbacks for OnChanOpenTry and OnChanOpenConfirm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not much reason beside adding more flexibility, might be useful for the wired app? if not it should return nil anyways?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@colin-axner I am not totally sure how I should handle the channel cap here. I moved it to the msg_server level. Is there anything else that needs to be done?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delayed reply! This looks good to me, although I'm not sure it matters if ics20 continues to claim the channel capability in the channel handshake callbacks.
@AdityaSripal Do we want to allow applications using ics20 as middleware to claim the capability?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thomas-nguy Is it desirable for your use case for the connected application to be capable of sending packets? Otherwise I'm thinking maybe we should leave all the capability management as it was. Essentially only allowing the connected application to get information about the ICS20 transfer, but not act upon it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessary, but I did claim here to preserve the original behavior.
Before it was claimed at "SendTransfer" level, but this line needs to be removed since the capability is now passed by parameter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you refer to this section @AdityaSripal ?
The capability here is claim by the msg_server which is outside the middleware layer. Not sure to understand which check you want to do