Add support for concurrent bridge transaction and evidence#96
Add support for concurrent bridge transaction and evidence#96TxCorpi0x merged 3 commits intochains/coreum-v0.50.xfrom
Conversation
ysv
left a comment
There was a problem hiding this comment.
@ysv reviewed 12 of 12 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @masihyeganeh and @miladz68)
database/schema/19-bridge.sql line 5 at r1 (raw file):
bridge_transaction ( id SERIAL NOT NULL PRIMARY KEY, unique_key TEXT NOT NULL UNIQUE,
have we run this migration on testnet / mainnet ?
if have done this, migration will fail and we will have to do this manually
modules/bridge/xrpl.go line 126 at r1 (raw file):
transaction := types.NewBridgeTransaction( &operationUniqueID, uniqueKey,
do I understand it correctly that we use string here because this column is string for
XRPL -> Coreum transfers, but int for Coreum->XRPL
modules/bridge/xrpl.go line 216 at r1 (raw file):
// the user initiated hash is the xrpl tx hash user_initiated_hash := toCoreum[hashAttribute]
do you use snake case here on purpose ?
types/bridge.go line 49 at r1 (raw file):
// OperationUniqueID is the operation unique ID of the transaction (it might be null if there are no pending operations). OperationUniqueID *string `json:"operation_unique_id"` // UniqueKey is the a unique key matching the transaction with evidence.
I suggest better description & name:
OnchainKey / OnchainUniqueKey
OnchainUniqueKey is the unique key present onchain matching a transaction with evidences.
It is equal to operation_id (number) for Coreum -> XRPL transfers.
It is equal to XRPL transaction hash (string) for XRPL -> Coreum transfers.
TxCorpi0x
left a comment
There was a problem hiding this comment.
Reviewable status: 5 of 12 files reviewed, 4 unresolved discussions (waiting on @masihyeganeh, @miladz68, and @ysv)
database/schema/19-bridge.sql line 5 at r1 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
have we run this migration on testnet / mainnet ?
if have done this, migration will fail and we will have to do this manually
Yes, according to the call, we decided to drop the tables, i have not put this in the main migration to prevent table deletion in case of accidental running of the migration scripts in the future.
modules/bridge/xrpl.go line 126 at r1 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
do I understand it correctly that we use string here because this column is string for
XRPL -> Coreum transfers, but int for Coreum->XRPL
Yes, XRPL Hash and Coreum OperationID/OperationUniqueID
modules/bridge/xrpl.go line 216 at r1 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
do you use snake case here on purpose ?
Done.
types/bridge.go line 49 at r1 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
I suggest better description & name:
OnchainKey/OnchainUniqueKeyOnchainUniqueKey is the unique key present onchain matching a transaction with evidences. It is equal to operation_id (number) for Coreum -> XRPL transfers. It is equal to XRPL transaction hash (string) for XRPL -> Coreum transfers.
Done
ysv
left a comment
There was a problem hiding this comment.
@ysv reviewed 7 of 7 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @masihyeganeh, @miladz68, and @TxCorpi0x)
database/schema/19-bridge.sql line 5 at r1 (raw file):
Previously, TxCorpi0x wrote…
Yes, according to the call, we decided to drop the tables, i have not put this in the main migration to prevent table deletion in case of accidental running of the migration scripts in the future.
well, I'm not sure what is the approach we should use here to manage DB migration. In all my BE projects I preferred to do everything via migrations which run automatically on each deployment in incremental fashion. So no manual scripts were run on DB and schema/state was same for all envs.
The rule was to never edit existing migration but to create new.
Not sure if migration tool in callisto manages migrations same way
Lets discuss in slack
TxCorpi0x
left a comment
There was a problem hiding this comment.
Reviewable status: 11 of 12 files reviewed, 1 unresolved discussion (waiting on @masihyeganeh, @miladz68, and @ysv)
database/schema/19-bridge.sql line 5 at r1 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
well, I'm not sure what is the approach we should use here to manage DB migration. In all my BE projects I preferred to do everything via migrations which run automatically on each deployment in incremental fashion. So no manual scripts were run on DB and schema/state was same for all envs.
The rule was to never edit existing migration but to create new.
Not sure if migration tool in callisto manages migrations same wayLets discuss in slack
There is a new section in the beginning of each create table statement, it checks if the old table exists, drops it
ysv
left a comment
There was a problem hiding this comment.
@ysv reviewed 1 of 7 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @masihyeganeh and @miladz68)
This pull request updates the schema and database logic for bridge transactions and evidence to use a new
unique_keyas the primary reference between the two tables, replacing the previous use of transaction IDs. It also removes the now-unnecessaryGetBridgeTransactionmethod and updates the GraphQL schema to reflect these changes. These updates improve referential integrity and simplify cross-table relationships.Database schema and relationships:
unique_keycolumn to thebridge_transactiontable, enforced as unique, and updated all related indexes accordingly.bridge_evidencetable to reference bridge transactions viatx_unique_key(a string) instead oftransaction_id(an integer foreign key), and added an index for efficient lookups. (database/schema/19-bridge.sql,Go database logic:
SaveBridgeTransactionandSaveBridgeEvidencemethods to handle the newunique_keyandtx_unique_keyfields, including updating SQL statements and method parameters.GetBridgeTransactionmethod, as lookups by operation unique ID are no longer needed.GraphQL schema updates:
transaction_idtotx_unique_keyin thebridge_evidencetype and related input/output types, constraints, and enums. Also made the relationship frombridge_evidencetobridge_transactionnullable to reflect the new linkage.unique_keyfield and related constraints to thebridge_transactiontype and input/output types. (hasura/api/schema.graphql,Other:
go.mod(cosmetic, no functional impact). (go.mod, go.modL41)This change is