Relayer: Sending of the coreum originated token from the coreum to XRPL.

[dzmitryhil]

Description

Relayer: Sending of the coreum originated token from the coreum to XRPL.

• Add contract client integration tests
• Implement relayer part for the sending

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 

[wojtek-coreum]

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

---

relayer/processes/amount.go line 66 at r1 (raw file):

// ConvertCoreumOriginatedTokenCoreumAmountToXRPLAmount converts the coreum originated token amount to XRPL amount based on decimals.
func ConvertCoreumOriginatedTokenCoreumAmountToXRPLAmount(coreumAmount sdkmath.Int, decimals uint32, issuerString, currencyString string) (rippledata.Amount, error) {

What's the point of this function? Can't convertCoreumAmountToXRPLAmountWithDecimals be exported?

---

relayer/processes/xrpl_tx_observer.go line 63 at r1 (raw file):

	if cfg.BridgeXRPLAddress != o.cfg.BridgeXRPLAddress.String() {
		return errors.Errorf(
			"observer bridge XRPL address in config is different form the contract %s!=%s",

form -> from

[dzmitryhil]

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

---

relayer/processes/amount.go line 66 at r1 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

What's the point of this function? Can't convertCoreumAmountToXRPLAmountWithDecimals be exported?

The convertCoreumAmountToXRPLAmountWithDecimals is reused for this func and ConvertXRPLOriginatedTokenCoreumAmountToXRPLAmount. In order to keep the naming clear I use the unexported function.

---

relayer/processes/xrpl_tx_observer.go line 63 at r1 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

form -> from

Done.

[ysv]

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

---

integration-tests/coreum/contract_client_test.go line 120 at r2 (raw file):

		Currency:         xrpCurrency,
		CoreumDenom:      coreumDenom,
		SendingPrecision: xrpSendingPrecision,

is it XRP sending precision ? or xrplOriginatedToken ?
Maybe we should rename it on contract side also ?

---

integration-tests/coreum/contract_client_test.go line 320 at r2 (raw file):

	maxHoldingAmount := sdk.NewIntFromUint64(10000)

	// recover tickets to be able to create operations form coreum to XRPL

typo

Also check other same places

Code quote:

form

---

integration-tests/coreum/contract_client_test.go line 1325 at r2 (raw file):

}

func TestSendFromCoreumToXRPLCoreumOriginatedToken(t *testing.T) {

nit:
Maybe we can improve readability of tests names by using _ ?
E.g here could be Test_SendFromCoreumToXRPL_CoreumOriginatedToken or smth else.
Because names are too long and complicated to read

---

integration-tests/coreum/contract_client_test.go line 1436 at r2 (raw file):

	operationType := operation.OperationType.CoreumToXRPLTransfer
	require.NotNil(t, operationType)
	require.NotNil(t, operationType.Issuer, bridgeXRPLAddress)

nit: are 3rd params expected to be here ? (they will be used as error message)

---

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

	if cfg.BridgeXRPLAddress != s.cfg.BridgeXRPLAddress.String() {
		return errors.Errorf(
			"submitter bridge XRPL address in config is different from the contract %s!=%s",

we do we need to have it in 2 places ? Can't we have a single source of truth - contract ?

---

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

}

func getContractRelayers(relayersCount int) ([]coreum.Relayer, []*xrpl.PrivKeyTxSigner, xrpl.AccountInfoResult) {

nit: maybe it should be genContractRelayers ?
Since you generate all the keys etc

---

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

}

func buildAllocateTicketsTestData(

I didn't get the logic you are trying to achieve in this func

Also in usage I can see this allocateTicketOperationValidSigners := buildAllocateTicketsTestData(t, xrplTxSigners, bridgeXRPLAddress, contractRelayers)

But you have 3 params returned from func

[dzmitryhil]

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

---

integration-tests/coreum/contract_client_test.go line 120 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

is it XRP sending precision ? or xrplOriginatedToken ?
Maybe we should rename it on contract side also ?

XRP sending precision

---

integration-tests/coreum/contract_client_test.go line 320 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

typo

Also check other same places

Done

---

integration-tests/coreum/contract_client_test.go line 1325 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

nit:
Maybe we can improve readability of tests names by using _ ?
E.g here could be Test_SendFromCoreumToXRPL_CoreumOriginatedToken or smth else.
Because names are too long and complicated to read

I'm ok with improving it, but don't see a big difference between snake-case and camel-case

---

integration-tests/coreum/contract_client_test.go line 1436 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

nit: are 3rd params expected to be here ? (they will be used as error message)

Did't get the question.

---

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

Previously, ysv (Yaroslav Savchuk) wrote…

we do we need to have it in 2 places ? Can't we have a single source of truth - contract ?

We have it here and in another place since we decided to have independent processes. For both those processes it is important to have the correct bridge address, that's why we validate them in both.

---

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

Previously, ysv (Yaroslav Savchuk) wrote…

nit: maybe it should be genContractRelayers ?
Since you generate all the keys etc

You are right, updated.

---

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

Previously, ysv (Yaroslav Savchuk) wrote…

I didn't get the logic you are trying to achieve in this func

Also in usage I can see this allocateTicketOperationValidSigners := buildAllocateTicketsTestData(t, xrplTxSigners, bridgeXRPLAddress, contractRelayers)

But you have 3 params returned from func

I have 4 params returned from it and I use all of them. All of the build**TestData functions are responsible for building the transaction arguments required for the test, and most of them are built with the same strategy.

[ysv]

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

---

integration-tests/coreum/contract_client_test.go line 120 at r2 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

XRP sending precision

ok
Shall we rename it ? not it this task but during the next refactoring to XRPSendingPrecision ?
Because I thought that it is precission which will be used for all xrpl tokens

---

integration-tests/coreum/contract_client_test.go line 1436 at r2 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Did't get the question.

if this check fails bridgeXRPLAddress will be shown as error.
Is it what you wanted here ?

Screenshot 2023-11-22 at 16.56.42

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

We have it here and in another place since we decided to have independent processes. For both those processes it is important to have the correct bridge address, that's why we validate them in both.

this is gonna be configured on relayer level (via flag or config file)
But I mean that we don't need it since it can only be equal to the value stored inside contract so it means we should fetch it from there for initialization. WDYT ?

[dzmitryhil]

Reviewable status: 18 of 24 files reviewed, 5 unresolved discussions (waiting on @miladz68, @wojtek-coreum, and @ysv)

---

integration-tests/coreum/contract_client_test.go line 120 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

ok
Shall we rename it ? not it this task but during the next refactoring to XRPSendingPrecision ?
Because I thought that it is precission which will be used for all xrpl tokens

This is the default precision we set in the contract for the XRP token which we issue at the instantiation time. It will be possible to change it later by admin.

---

integration-tests/coreum/contract_client_test.go line 1436 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

if this check fails bridgeXRPLAddress will be shown as error.
Is it what you wanted here ?

Screenshot 2023-11-22 at 16.56.42

Got you, fixed.

---

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

Previously, ysv (Yaroslav Savchuk) wrote…

this is gonna be configured on relayer level (via flag or config file)
But I mean that we don't need it since it can only be equal to the value stored inside contract so it means we should fetch it from there for initialization. WDYT ?

Right, got you. Updated to fetch the address from the contract in the runner. And removed that check from the init.

[ysv]

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

[wojtek-coreum]

Reviewed 1 of 7 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @miladz68)

[wojtek-coreum]

Reviewed 3 of 7 files at r2, 3 of 4 files at r3, 6 of 7 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @miladz68)