SC-7142 envelope id and error return #105
Conversation
pkg/rvasp/rvasp.go
Outdated
| case *protocol.Error: | ||
| log.Warn().Str("message", err.Error()).Msg("TRISA protocol error while performing transfer") | ||
| reply.Error = &pb.Error{ | ||
| Message: err.Error(), |
There was a problem hiding this comment.
I tried to put the error code in here instead of using Error() but apparently it wasn't being populated on envelope.Reject()?
There was a problem hiding this comment.
It's not? That's not good - let me take a look at the TRISA code.
There was a problem hiding this comment.
It looks like Reject is just adding the error to the struct without modification:
https://github.com/trisacrypto/trisa/blob/main/pkg/trisa/envelope/envelope.go#L136-L149
I guess it's up to the client to return an error that has an error code? Is there somewhere in the rVASP code where we're forgetting to add a code?
There was a problem hiding this comment.
Yes there was a spot where the rVASP was not returning the error code back which I fixed in the previous push but I forgot to update the code here.
There was a problem hiding this comment.
@pdeziel looks great - I agree that a smoke test with Charlie would be appropriate.
I also received one additional request today; could we change the Transaction envelope field to envelope_id:
https://github.com/trisacrypto/testnet/blob/main/proto/rvasp/v1/api.proto#L47
There was some confusion about that field. Although it's not best practice to rename fields, I don't think this will affect any code outside of the rvasps and the cli should still be backwards compatible since all it does is print out the json of the transfer reply.
pkg/rvasp/rvasp.go
Outdated
| reply.Error = &pb.Error{ | ||
| Message: err.Error(), | ||
| } |
There was a problem hiding this comment.
Could we also include the code here as well?
| reply.Error = &pb.Error{ | |
| Message: err.Error(), | |
| } | |
| reply.Error = &pb.Error{ | |
| Code: err.Code, | |
| Message: err.Message, | |
| } |
| @@ -371,8 +382,7 @@ func (s *Server) sendTransfer(xfer *db.Transaction, beneficiary *db.Wallet, part | |||
| reject, isErr := envelope.Check(msg) | |||
| if isErr { | |||
| if reject != nil { | |||
| xfer.SetState(pb.TransactionState_REJECTED) | |||
| return nil | |||
| return reject | |||
There was a problem hiding this comment.
Confirming from above comment that it is returning the rejection error as an error.
| @@ -80,7 +80,7 @@ message TransferRequest { | |||
| // or an error if there are insufficient funds or the account or beneficiary could not | |||
| // be looked up. Errors encountered during the TRISA protocol may also be returned. | |||
| message TransferReply { | |||
| Error error = 1; // Only used in live stream | |||
| Error error = 1; // populated with an error encountered during the transfer or from the response envelope | |||
This fixes two rVASP issues: One where the envelope id was being recreated and one where the error field was not being populated in the response to a user API request.