sc-5933 Marshal transaction #99
Conversation
| Envelope string `gorm:"not null"` | ||
| AccountID uint `gorm:"not null"` | ||
| Account Account `gorm:"foreignKey:AccountID"` | ||
| OriginatorID uint `gorm:"column:originator_id;not null"` | ||
| Originator Identity `gorm:"foreignKey:OriginatorID"` | ||
| BeneficiaryID uint `gorm:"column:beneficiary_id;not null"` | ||
| Beneficiary Identity `gorm:"foreignKey:BeneficiaryID"` | ||
| Amount decimal.Decimal `gorm:"type:numeric(15,2)"` | ||
| Amount decimal.Decimal `gorm:"type:decimal(15,8)"` |
There was a problem hiding this comment.
To avoid truncation in the database for small valued transfers.
There was a problem hiding this comment.
Hehe, you're right numeric(15,2) is for real money that has cents and not partial cents but we definitely need more precision for bitcoin transactions that can have a large fraction of a coin! Definitely a bit of bias we added when I first developed this model!
| else | ||
| ALICE_ENDPOINT=admin.alice.vaspbot.net:443 | ||
| BOB_ENDPOINT=admin.bob.vaspbot.net:443 | ||
| fi |
There was a problem hiding this comment.
I've discovered that this script is really useful for local testing as well, thanks for adding it!
| Envelope string `gorm:"not null"` | ||
| AccountID uint `gorm:"not null"` | ||
| Account Account `gorm:"foreignKey:AccountID"` | ||
| OriginatorID uint `gorm:"column:originator_id;not null"` | ||
| Originator Identity `gorm:"foreignKey:OriginatorID"` | ||
| BeneficiaryID uint `gorm:"column:beneficiary_id;not null"` | ||
| Beneficiary Identity `gorm:"foreignKey:BeneficiaryID"` | ||
| Amount decimal.Decimal `gorm:"type:numeric(15,2)"` | ||
| Amount decimal.Decimal `gorm:"type:decimal(15,8)"` |
There was a problem hiding this comment.
Hehe, you're right numeric(15,2) is for real money that has cents and not partial cents but we definitely need more precision for bitcoin transactions that can have a large fraction of a coin! Definitely a bit of bias we added when I first developed this model!
| @@ -407,7 +407,7 @@ | |||
| "badnews@evilvasp.gg", | |||
| 3, | |||
| "SendFull", | |||
| "SyncRequire", | |||
| "AsyncRepair", | |||
There was a problem hiding this comment.
Will this require us to re-migrate and update the documentation?
There was a problem hiding this comment.
The documentation is already updated but we do need to run the migration at some point.
There was a problem hiding this comment.
Is there a good stopping point that we can tag this as v1.0 to test goreleaser and the container builds then run the migrations? The postgres migrations shouldn't result in any downtime for the rVASPs, so I think we can run them when we need to; but we should still do it at off-peak hours.
There was a problem hiding this comment.
We can do that whenever, I don't have any more rVASP work planned for the current sprint.
| @@ -524,8 +524,7 @@ func (s *Server) respondAsync(peer *peers.Peer, payload *protocol.Payload, ident | |||
| switch xfer.State { | |||
| case pb.TransactionState_AWAITING: | |||
| // Fill the transaction with a new TxID to continue the handshake | |||
| xfer.TxID = uuid.New().String() | |||
| transaction.Txid = xfer.TxID | |||
| transaction.Txid = uuid.New().String() | |||
There was a problem hiding this comment.
Good catch - CipherTrace has been reporting an issue where the rVASP returns an incorrect "receipt ID" than the one it was sent. I wonder if this is related. Note that they claim that receipt ID == envelope ID, but I'm not sure about that terminology. CC @DanielSollis
| Identity string `gorm:"not null"` | ||
| Transaction string `gorm:"not null"` |
There was a problem hiding this comment.
Now that we're using postgres we could consider making these JSON types rather than raw strings -- but only if it's easy to do.
| else | ||
| ALICE_ENDPOINT=admin.alice.vaspbot.net:443 | ||
| BOB_ENDPOINT=admin.bob.vaspbot.net:443 | ||
| fi |
This marshals the
generic.Transactionpayload as a string to the database in a similar manner to how we're handling the identity payload. This allows us to avoid having to reconstruct the payload for async transactions so the rvasps can act a bit more consistent. I also discovered some bugs in local testing which I've taken the opportunity to fix.