Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add trace sending CLI #204

Merged
merged 8 commits into from
Apr 12, 2024
Merged

Add trace sending CLI #204

merged 8 commits into from
Apr 12, 2024

Conversation

dzmitryhil
Copy link
Contributor

@dzmitryhil dzmitryhil commented Apr 8, 2024

Description

Add trace sending CLI

Reviewers checklist:

  • Try to write more meaningful comments with clear actions to be taken.
  • Nit-picking should be unblocking. Focus on core issues.

Authors checklist

  • Provide a concise and meaningful description
  • Review the code yourself first, before making the PR.
  • Annotate your PR in places that require explanation.
  • Think and try to split the PR to smaller PR if it is big.

This change is Reviewable

@dzmitryhil dzmitryhil requested a review from a team as a code owner April 8, 2024 11:55
@dzmitryhil dzmitryhil requested review from masihyeganeh, miladz68, ysv and wojtek-coreum and removed request for a team April 8, 2024 11:55
@dzmitryhil dzmitryhil force-pushed the dzmitryhil/trace-sending-cli branch from ffd374e to 59ffab0 Compare April 8, 2024 12:17
Copy link
Collaborator

@ysv ysv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 15 files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @masihyeganeh, @miladz68, and @wojtek-coreum)


relayer/coreum/contract.go line 1532 at r1 (raw file):

	}

	operationIDs, err := c.getUsedOperationIDsForTx(ctx, tx.Height)

what if here you use PendingOperations query instead of AvailableTickets.

The flow:

  1. query PendingOperations by height-1 and by height
  2. select all new operations (ones which exist in height but don't exist in heigh-1).
  3. since PendingOperation of needed type contains CoreumToXRPLTransfer inside. You can compare if sender, recipient, currency and optionally amount matches values in your tx. This way you have 99.9% probability of finding 1 operation which you need. This approach is better than tickets IMO.
CoreumToXRPLTransfer {  
issuer: String,  
currency: String,  
amount: Uint128,  
max\_amount: Option<Uint128>,  
sender: Addr,  
recipient: String,  
}
  1. PendingOperation also contains id. You can use this id same way to query txs by events. Here I have a question (why your id is just ticket_sequence while usually it is {timestamp}-{ticket-sequence} ?

Copy link
Contributor Author

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 15 files reviewed, 1 unresolved discussion (waiting on @masihyeganeh, @miladz68, @wojtek-coreum, and @ysv)


relayer/coreum/contract.go line 1532 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

what if here you use PendingOperations query instead of AvailableTickets.

The flow:

  1. query PendingOperations by height-1 and by height
  2. select all new operations (ones which exist in height but don't exist in heigh-1).
  3. since PendingOperation of needed type contains CoreumToXRPLTransfer inside. You can compare if sender, recipient, currency and optionally amount matches values in your tx. This way you have 99.9% probability of finding 1 operation which you need. This approach is better than tickets IMO.
CoreumToXRPLTransfer {  
issuer: String,  
currency: String,  
amount: Uint128,  
max\_amount: Option<Uint128>,  
sender: Addr,  
recipient: String,  
}
  1. PendingOperation also contains id. You can use this id same way to query txs by events. Here I have a question (why your id is just ticket_sequence while usually it is {timestamp}-{ticket-sequence} ?

Agree, updated.

miladz68
miladz68 previously approved these changes Apr 9, 2024
Copy link
Contributor

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 10 of 15 files at r1, 5 of 5 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @masihyeganeh, @wojtek-coreum, and @ysv)

Copy link
Collaborator

@ysv ysv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 10 of 15 files at r1, 5 of 5 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dzmitryhil, @masihyeganeh, and @wojtek-coreum)


relayer/client/bridge.go line 247 at r3 (raw file):

type XRPLToCoreumTracingInfo struct {
	XRPLTx        rippledata.TransactionWithMetaData
	CoreumTx      *sdk.TxResponse

nit: why do we have *sdk.TxResponse here and sdk.TxResponse non-pointer in CoreumToXRPLTracingInfo ? is there a specific reason ?

Code quote:

CoreumTx      *sdk.TxResponse

relayer/cmd/cli/coreum_cli.go line 1032 at r3 (raw file):

				components.Log.Info(
					ctx,
					"Transfer is complete for XRPL txs.",

how do you handle the situation when you have multiple bridge msgs inside single tx ?
I would probably just return error in such case since it is not usual usage & frankly speaking I doubt if smn would do this

Code quote:

"Transfer is complete for XRPL txs.",

relayer/coreum/contract.go line 1889 at r3 (raw file):

				return true
			}
			return false

nit: u can just put return attr.Value == value instead of these 4 lines

Code quote:

			if attr.Value == value {
				return true
			}
			return false

Copy link
Contributor Author

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @masihyeganeh, @wojtek-coreum, and @ysv)


relayer/client/bridge.go line 247 at r3 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

nit: why do we have *sdk.TxResponse here and sdk.TxResponse non-pointer in CoreumToXRPLTracingInfo ? is there a specific reason ?

* is for optional attributes


relayer/cmd/cli/coreum_cli.go line 1032 at r3 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

how do you handle the situation when you have multiple bridge msgs inside single tx ?
I would probably just return error in such case since it is not usual usage & frankly speaking I doubt if smn would do this

We handle it, since the event logs are grouped per message in the transaction, so one Coreum transaction can contain multiple XRPL transactions. And even an integration test for the CLI produces such a situation.


relayer/coreum/contract.go line 1889 at r3 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

nit: u can just put return attr.Value == value instead of these 4 lines

Done

Copy link
Collaborator

@ysv ysv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @masihyeganeh and @wojtek-coreum)

Copy link
Contributor

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @masihyeganeh and @wojtek-coreum)

@dzmitryhil dzmitryhil merged commit 2534b82 into master Apr 12, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants