Relayer: Sending of XRPL originated toke from the coreum back to the XRPL implementation

[dzmitryhil]

Relayer: Sending of XRPL originated toke from the coreum back to the XRPL implementation

• Update client and add integration test to all new functionality
• Integrate the relayer submitter Payment (CoremToXRPL) transaction
• Integrate relayer observer Payment (CoremToXRPL) confirmation

Description

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 

[ysv]

Reviewed 4 of 19 files at r1, all commit messages.
Reviewable status: 4 of 19 files reviewed, 4 unresolved discussions (waiting on @dzmitryhil, @miladz68, and @wojtek-coreum)

a discussion (no related file):
Reviewed 1st part will continue review tomorrow

---

---

integration-tests/coreum/contract_client_test.go line 692 at r1 (raw file):

			_, err := contractClient.RegisterXRPLToken(ctx, owner, issuer, currency, tt.sendingPrecision, tt.maxHoldingAmount)
			require.NoError(t, err)
			registerXRPLToken, err := contractClient.GetXRPLToken(ctx, issuer, currency)

nit: register -> registered ?

---

integration-tests/coreum/contract_client_test.go line 1128 at r1 (raw file):

	require.Empty(t, pendingOperations)

	// use all available tickets

nit: use all available tickets except the last one

---

integration-tests/coreum/contract_client_test.go line 1297 at r1 (raw file):

}

func allocateInitialTickets(

question:
is it the same as ticket recovery flow ?

[dzmitryhil]

Reviewable status: 4 of 19 files reviewed, 3 unresolved discussions (waiting on @miladz68, @wojtek-coreum, and @ysv)

a discussion (no related file):

Previously, ysv (Yaroslav Savchuk) wrote…

Reviewed 1st part will continue review tomorrow

OK

---

---

integration-tests/coreum/contract_client_test.go line 692 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

nit: register -> registered ?

Done

---

integration-tests/coreum/contract_client_test.go line 1128 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

nit: use all available tickets except the last one

The last one is not available.

---

integration-tests/coreum/contract_client_test.go line 1297 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

question:
is it the same as ticket recovery flow ?

Yes, we use it for the recovery in case of the failure and for the initial bootstrapping as well.

[ysv]

Reviewed 15 of 19 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @dzmitryhil, @miladz68, and @wojtek-coreum)

a discussion (no related file):
in general code looks a bit heavy to ready because of long names
e.g

xrplTransactionEvidenceCoreumToXRPLTransferOperationResult  
SendCoreumToXRPLTransferTransactionResultEvidence

and there are plenty of places with such long names. They are mostly related to smart contract by the way

Do you have any ideas of how this could be improved ?

The approaches I can think about are:

1. to shorten Transaction to Tx.
2. Maybe we can replace all places where we mention CoreumToXRPL or XRPLToCoreum to just CoreumIn/CoreumOut. (Not sure)

Do you have any ideas ? IMO this shortening would make code significantly more redable

---

---

integration-tests/coreum/contract_client_test.go line 1128 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

The last one is not available.

I meant to adjust the comment

---

integration-tests/coreum/contract_client_test.go line 1297 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Yes, we use it for the recovery in case of the failure and for the initial bootstrapping as well.

then a bit better name would be recoverTickets but I'm ok with current also

---

integration-tests/processes/env_test.go line 332 at r2 (raw file):

func (r *RunnerEnv) EnableXRPLAccountRippling(ctx context.Context, t *testing.T, account rippledata.Account) {
	// enable rippling on this account's trust lines.

is it about this config https://xrpl.org/rippling.html#the-default-ripple-flag ?

---

integration-tests/processes/send_from_coreum_to_xrpl_test.go line 23 at r2 (raw file):

)

func TestRegisterXRPLOriginatedTokensSendFromXRPLToCoreumAndBack(t *testing.T) {

nit:
TestRegister...
Is register name needed here ?

---

integration-tests/processes/send_from_coreum_to_xrpl_test.go line 97 at r2 (raw file):

	runnerEnv.AllocateTickets(ctx, t, uint32(200))

	coreumSender := chains.Coreum.GenAccount()

I can see that you receive tokens to this address at first
Shouldn't it be coreumReceiver

---

integration-tests/processes/send_from_xrpl_to_coreum_test.go line 133 at r2 (raw file):

}

func TestSendFromXRPLToCoreumXRPLOriginatedWithMaliciousRelayer(t *testing.T) {

as far as I understand you send to coreum and back here ?

---

relayer/processes/xrpl_tx_submitter.go line 414 at r2 (raw file):

		operation.OperationType.TrustSet.Currency != "":
		return BuildTrustSetTxForMultiSigning(s.cfg.BridgeAccount, operation)
	case operation.OperationType.CoreumToXRPLTransfer != nil &&

I would move these validations into 2 separate methods because doing all this checks in switch case doesn't look beatifull

smth like

isValidTrustSetOperation 
isValidCoreumToXRPLTransferOperation

---

relayer/processes/xrpl_tx_submitter_test.go line 19 at r2 (raw file):

)

func TestXRPLTxSubmitter_Start(t *testing.T) {

TestXRPLTxSubmitter_Start is getting a bit too big
Maybe you want to split it into multiple by functionality: allocate tickets, trust set etc

---

relayer/processes/xrpl_tx_submitter_test.go line 376 at r2 (raw file):

		},
		{
			name: "resister_signature_for_coreum_to_XRPL_transfer_payment_tx",

typo: register

Code quote:

resister

[dzmitryhil]

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

a discussion (no related file):

Previously, ysv (Yaroslav Savchuk) wrote…

in general code looks a bit heavy to ready because of long names
e.g

xrplTransactionEvidenceCoreumToXRPLTransferOperationResult  
SendCoreumToXRPLTransferTransactionResultEvidence

and there are plenty of places with such long names. They are mostly related to smart contract by the way

Do you have any ideas of how this could be improved ?

The approaches I can think about are:

1. to shorten Transaction to Tx.
2. Maybe we can replace all places where we mention CoreumToXRPL or XRPLToCoreum to just CoreumIn/CoreumOut. (Not sure)

Do you have any ideas ? IMO this shortening would make code significantly more redable

The problem here is that you work with similar things but in different areas.
E.g. XRPL token, Coreum token, XRPL user, Coreum user and etc. And when have an operation that connects 2 arias the name will be always long. I would love to rename it, but in that case, we will lose the readability as well since you will have to keep in mind the naming partners written code at the same time.

---

---

integration-tests/coreum/contract_client_test.go line 1128 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

I meant to adjust the comment

I know, I mean the comment is correct.

---

integration-tests/coreum/contract_client_test.go line 1297 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

then a bit better name would be recoverTickets but I'm ok with current also

Done

---

integration-tests/processes/env_test.go line 332 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

is it about this config https://xrpl.org/rippling.html#the-default-ripple-flag ?

Yes

---

integration-tests/processes/send_from_coreum_to_xrpl_test.go line 23 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

nit:
TestRegister...
Is register name needed here ?

We test the token registration here as well.

---

integration-tests/processes/send_from_coreum_to_xrpl_test.go line 97 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

I can see that you receive tokens to this address at first
Shouldn't it be coreumReceiver

We receive to this address and send from it, and the test intention is to test the send, that's why I call it sender.

---

integration-tests/processes/send_from_xrpl_to_coreum_test.go line 133 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

as far as I understand you send to coreum and back here ?

Here - not. Only one way.

---

relayer/processes/xrpl_tx_submitter.go line 414 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

I would move these validations into 2 separate methods because doing all this checks in switch case doesn't look beatifull

smth like

isValidTrustSetOperation 
isValidCoreumToXRPLTransferOperation

Agree, done.

---

relayer/processes/xrpl_tx_submitter_test.go line 19 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

TestXRPLTxSubmitter_Start is getting a bit too big
Maybe you want to split it into multiple by functionality: allocate tickets, trust set etc

The test cases are good, but the arguments take space. Moving it to different place will lead to code duplication of the for_, tt := range tests {... part, and additional support of it. Let's agreed that next time I modify it, I'll move the args creation to specific functions.

---

relayer/processes/xrpl_tx_submitter_test.go line 376 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

typo: register

Fixed

[ysv]

Reviewed 4 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @miladz68, and @wojtek-coreum)

a discussion (no related file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

The problem here is that you work with similar things but in different areas.
E.g. XRPL token, Coreum token, XRPL user, Coreum user and etc. And when have an operation that connects 2 arias the name will be always long. I would love to rename it, but in that case, we will lose the readability as well since you will have to keep in mind the naming partners written code at the same time.

ok, but Tx should be fine IMO
It is well known short version

not necessary to rename it right now we can keep it for future cleanup/refactoring

---

[dzmitryhil]

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

a discussion (no related file):

Previously, ysv (Yaroslav Savchuk) wrote…

ok, but Tx should be fine IMO
It is well known short version

not necessary to rename it right now we can keep it for future cleanup/refactoring

Let's discuss it today.

---

[ysv]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @miladz68 and @wojtek-coreum)

a discussion (no related file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Let's discuss it today.

approved for now
lets merge it as it is and we can discusss later

---

[miladz68]

Reviewed 15 of 19 files at r1, 4 of 4 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @wojtek-coreum)

[wojtek-coreum]

Reviewed 15 of 19 files at r1, 4 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dzmitryhil)

---

integration-tests/xrpl.go line 255 at r3 (raw file):

}

// GetAccountBalance returns account balance for the provides issuer and currency.

provides?

---

integration-tests/coreum/contract_client_test.go line 1277 at r3 (raw file):

			if tt.wantIsAmountSentIsZeroAfterTruncationError {
				require.True(t, coreum.IsAmountSentIsZeroAfterTruncationError(err), err)
				return

you don't need this

---

relayer/processes/xrpl_tx_submitter.go line 415 at r3 (raw file):

		return BuildCoreumToXRPLTransferPaymentTxForMultiSigning(s.cfg.BridgeAccount, operation)
	default:
		return nil, errors.Errorf("failed to process operation, unable to determin operation type, operation:%+v", operation)

determine

[dzmitryhil]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @wojtek-coreum)

---

integration-tests/xrpl.go line 255 at r3 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

provides?

Done.

---

integration-tests/coreum/contract_client_test.go line 1277 at r3 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

you don't need this

removed

---

relayer/processes/xrpl_tx_submitter.go line 415 at r3 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

determine

Done.

[ysv]

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @wojtek-coreum)

[ysv]

Reviewed 4 of 4 files at r5, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @miladz68 and @wojtek-coreum)

[wojtek-coreum]

Reviewed 2 of 3 files at r4, 3 of 4 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dzmitryhil and @miladz68)

---

integration-tests/coreum/contract_client_test.go line 1277 at r3 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

removed

I still see return here

[dzmitryhil]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @miladz68 and @wojtek-coreum)

---

integration-tests/coreum/contract_client_test.go line 1277 at r3 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

I still see return here

We need return, actually, since we don't need to processed after this check.

[dzmitryhil]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @miladz68 and @wojtek-coreum)

---

integration-tests/coreum/contract_client_test.go line 1277 at r3 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

We need return, actually, since we don't need to processed after this check.

Actually removed it first time and later returned it back

[wojtek-coreum]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @miladz68)

[miladz68]

Reviewed 2 of 3 files at r4, 3 of 4 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dzmitryhil)