SC-6043 Improve error handling #102
Conversation
| @@ -260,6 +261,7 @@ type Transaction struct { | |||
| Amount decimal.Decimal `gorm:"type:decimal(15,8)"` | |||
| Debit bool `gorm:"not null"` | |||
| State pb.TransactionState `gorm:"not null;default:0"` | |||
| StateString string `gorm:"column:state_string;not null"` | |||
There was a problem hiding this comment.
Added this so we see the actual transaction states from psql. My verification process for local development is to now run the test script, then query select amount, vasp_id, state_string, id, envelope from transactions order by amount; to get a list of all the transactions; they should be either COMPLETED or REJECTED.
| @@ -58,7 +58,8 @@ enum TransactionState { | |||
| ACCEPTED = 4; // (Async) Originator has received the transaction acknowledgement from the beneficiary | |||
| FAILED = 5; // The transaction has failed | |||
| EXPIRED = 6; // The asynchronous transaction has expired before completion | |||
| COMPLETED = 7; // The transaction is completed | |||
| REJECTED = 7; // The transaction has been rejected | |||
| COMPLETED = 8; // The transaction is completed | |||
There was a problem hiding this comment.
The convention I'm using for the transaction states in the database is that intentional failures from the rVASPs are rejections (e.g., the beneficiary info wasn't correctly filled in), and miscommunication with the counterparty or internal errors are failures.
There was a problem hiding this comment.
I think this makes sense.
Note that because of the numbering change, everything in the database that used to be completed will now be rejected; therefore when we deploy this code a database migration is required. We should probably note that to ensure we take that step when we deploy.
CC @kbelita
There was a problem hiding this comment.
Having the rejected state is definitely going to make verification easier and like you suggest, perhaps we can run an integration test that verifies the state of transactions in the database; that would be really handy!
I read through all the code and everything makes sense to me. Unfortunately, I couldn't help verify that all the places that needed to be updated were updated since the change affected so much code already. If you'd like to smoke test by manually running some scenarios including full back-and-forth async before we deploy; let me know!
| var originatorIdentity, beneficiaryIdentity Identity | ||
|
|
||
| // Fetch originator identity record | ||
| if err := d.LookupIdentity(originator).FirstOrInit(&originatorIdentity, Identity{}).Error; err != nil { | ||
| log.Error().Err(err).Msg("could not lookup originator identity") | ||
| return Transaction{}, status.Errorf(codes.FailedPrecondition, "could not lookup originator identity: %s", err) | ||
| return nil, status.Errorf(codes.FailedPrecondition, "could not lookup originator identity: %s", err) |
There was a problem hiding this comment.
That is a little bit nicer - especially if you're checking nil (which is easier than checking iszero)
Another pattern if you're not checking nil that I like to do is:
func (d *DB) MakeTransaction(originator string, beneficiary string) (tx Transaction, err error) {
return tx, errors.New("this is an error, but tx is guaranteed to be zero-valued")
}There was a problem hiding this comment.
Although now that I've read farther in the code, I guess the real reason for the pointer is to make sure the transaction is editable?
There was a problem hiding this comment.
Correct, although the consistent nil return was an additional benefit.
| @@ -58,7 +58,8 @@ enum TransactionState { | |||
| ACCEPTED = 4; // (Async) Originator has received the transaction acknowledgement from the beneficiary | |||
| FAILED = 5; // The transaction has failed | |||
| EXPIRED = 6; // The asynchronous transaction has expired before completion | |||
| COMPLETED = 7; // The transaction is completed | |||
| REJECTED = 7; // The transaction has been rejected | |||
| COMPLETED = 8; // The transaction is completed | |||
There was a problem hiding this comment.
I think this makes sense.
Note that because of the numbering change, everything in the database that used to be completed will now be rejected; therefore when we deploy this code a database migration is required. We should probably note that to ensure we take that step when we deploy.
CC @kbelita
This improves a lot of the error handling by adding a
REJECTEDstate and making sure we write the transaction to the database before returning a response. This makes the rVASP verification process a lot easier, because we can simply look at the database after running the test script and see what states the transactions are in.