-
Notifications
You must be signed in to change notification settings - Fork 53
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
fix: correct validation for trust relationship request #476
Conversation
Can you fix the tests? @pranavkparti |
It seems to be some issue with the workflow itself. @samwel141 's PR also fails with the same errors.
|
// originating wallet doesn't need to send requests to a sub wallet it manages | ||
if (hasControlOverTarget) { | ||
if (actorHasControlOverTarget) { |
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.
Why was this replaced with actorHasControlOverTarget? @pranavkparti
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.
O = Originator, A = Actor, T = Target
By the time the code reaches here, the check is made that O != A && O != T && O has control over both A and T. Meaning that A and T are subwallets of O, and an error displays if it is true.
Next a check is made that A has control over T. This is to deal with the case if A = O, and has control over T. Meaning T (requestee) is already managed by A (requester).
Next, the last check is made that T = O. Meaning that A is a subwallet of O, and target is the originating wallet. Meaning T (requestee) already manages A (requester).
@Kpoke The checks are mainly for displaying specific and appropriate error messages.
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.
Okay noted. That's fine. I just wanted to confirm
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.
Can you pull the latest changes I made to your PR? It'll enable the PR github action run
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.
@Kpoke Done, thank you
🎉 This PR is included in version 1.40.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Description
Fixed validation while creating new trust relationship request.
What kind of change(s) does this PR introduce?
Please check if the PR fulfils these requirements
Breaking change
Does this PR introduce a breaking change?
No
Other useful information
None