SC-7425 Improve beneficiary identity check #109
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pdeziel
commented
Aug 3, 2022
| @@ -248,6 +246,9 @@ func (s *Server) Transfer(ctx context.Context, req *pb.TransferRequest) (reply * | |||
| } | |||
| } | |||
|
|
|||
| // Populate the transfer response with the transaction details | |||
| reply.Transaction = xfer.Proto() | |||
There was a problem hiding this comment.
This is just an ordering change so that we populate the transfer response for the CLI with the correct transaction state.
pdeziel
commented
Aug 3, 2022
| @@ -420,7 +420,8 @@ func (s *TRISA) handleTransaction(ctx context.Context, peer *peers.Peer, in *pro | |||
| return nil, protocol.Errorf(protocol.InternalError, "unknown policy '%s' for wallet '%s'", policy, account.WalletAddress) | |||
| } | |||
|
|
|||
| if transferError != nil { | |||
| // Mark transaction as failed if it was not rejected but an error occurred | |||
| if xfer.State != pb.TransactionState_REJECTED && transferError != nil { | |||
There was a problem hiding this comment.
This doesn't impact how the rVASPs operate but it does make sure that the transaction state gets updated correctly in the database, which makes it easier to manually verify our "integration" tests.
rebeccabilbro
approved these changes
Aug 5, 2022
There was a problem hiding this comment.
Excellent precision-fix here @pdeziel; this will absolutely help make the behavior of our robot vasps more consistent for our users. Great work!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This fixes an rVASP -> rVASP issue concerning the
SyncRequirepolicy.On the originator side, the rVASP generates an identity payload with a non-nil (but empty) beneficiary identity. However, on the beneficiary side the rVASP validates that the beneficiary identity is non-nil. This was causing an issue on
SendPartial->SyncRequirebecause the originating rVASP is generating an empty payload but the beneficiary rVASP assumes this is valid.To fix this, the validation check is improved to validate the actual fields within the beneficiary identity on the identity payload. The validation now uses the TRISA ivms methods to verify the
LegalPerson(the beneficiary VASP) andNaturalPerson(the beneficiary account).