sending precisions implementation for XRPLtoCoreumTransfers

[keyleu]

Description

• Changed hexadecimal currency validation back to Hex representation instead of string.
• Added validation of relayer configurations (relayers can't share addresses or pubkeys).
• Added check to avoid overwriting information in PENDING_OPERATIONS.
• Added send precision and max holding amount (Name to be discussed since it doesn't make sense as we are not holding anything, we are checking the supply) functionality.

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 

[dzmitryhil]

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

---

contract/src/contract.rs line 50 at r1 (raw file):

// Initial values for the XRP token that can be modified afterwards.
const XRP_SENDING_PRECISION: i32 = 6;

XRP_SENDING_PRECISION -> XRP_DEFAULT_SENDING_PRECISION (same for the amount)

---

contract/src/contract.rs line 120 at r1 (raw file):

        sending_precision: XRP_SENDING_PRECISION,
        max_holding_amount: Uint128::new(XRP_MAX_HOLDING_AMOUNT),
        min_sendable_amount,

You don't need to keep the min_sendable_amount on the contract, since the contract should compute each time it receives a transfer, and if after the routing the amount is zero return error. Also we use the rounded amount as a sendable amount not the original. Also, I would call it just sendable_amountbecase the amount higher than this might be rounded as well.
So based on what I've described theXRPLToCoreumTransfer handler should be updated.

---

contract/src/contract.rs line 672 at r1 (raw file):

}

fn calculate_min_sendable_amount(

1. The func shouldn't compute min amount it should truncate the amount
2. This function definitely requires unit tests. You can take as an example those tests: https://github.com/CoreumFoundation/coreumbridge-xrpl/blob/master/relayer/fee/rounding_test.go

---

contract/src/tests.rs line 108 at r1 (raw file):

            coreum_address: Addr::unchecked(signer.address()),
            xrpl_address: "rf1BiGeXwwQoi8Z2ueFYTEXSwuJYfV2Jpn".to_string(),
            xrpl_pub_key: "aBRNH5wUurfhZcoyR6nRwDSa95gMBkovBJ8V4cp1C1pM28H7EPL2".to_string(),

Don't you want to implement a func which generates it (same for all account, pubkey, signatures and etc)?

---

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

		coreum.Relayer{
			CoreumAddress: chains.Coreum.GenAccount(),
			XRPLAddress:   "xrpl_address",

Don't update it please, I've updated them all in my PR.

[keyleu]

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

---

contract/src/contract.rs line 50 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

XRP_SENDING_PRECISION -> XRP_DEFAULT_SENDING_PRECISION (same for the amount)

Changed.

---

contract/src/contract.rs line 120 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

You don't need to keep the min_sendable_amount on the contract, since the contract should compute each time it receives a transfer, and if after the routing the amount is zero return error. Also we use the rounded amount as a sendable amount not the original. Also, I would call it just sendable_amountbecase the amount higher than this might be rounded as well.
So based on what I've described theXRPLToCoreumTransfer handler should be updated.

Let's discuss it.

---

contract/src/contract.rs line 672 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

1. The func shouldn't compute min amount it should truncate the amount
2. This function definitely requires unit tests. You can take as an example those tests: https://github.com/CoreumFoundation/coreumbridge-xrpl/blob/master/relayer/fee/rounding_test.go

Let's discuss it.

---

contract/src/tests.rs line 108 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Don't you want to implement a func which generates it (same for all account, pubkey, signatures and etc)?

You mean generate a func to generate the structure passing the values? I don't so, won't change much.

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

Don't update it please, I've updated them all in my PR.

OK, changed it back.

[miladz68]

Reviewed 8 of 11 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @dzmitryhil, @wojtek-coreum, and @ysv)

[keyleu]

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

---

contract/src/contract.rs line 120 at r1 (raw file):

Previously, keyleu (Keyne) wrote…

Let's discuss it.

Fixed it, removed this.

---

contract/src/contract.rs line 672 at r1 (raw file):

Previously, keyleu (Keyne) wrote…

Let's discuss it.

Fixed this, removed and applied the truncating like we discussed.

[dzmitryhil]

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

---

contract/src/contract.rs line 50 at r1 (raw file):

Previously, keyleu (Keyne) wrote…

Changed.

Don't see the changes for the amount.

---

contract/src/contract.rs line 49 at r3 (raw file):

// Initial values for the XRP token that can be modified afterwards.
const XRP_DEFAULT_SENDING_PRECISION: i32 = 6;
const XRP_MAX_HOLDING_AMOUNT: u128 = 10u128.pow(16);

Did you compute this using the spec and recommended value?

---

contract/src/contract.rs line 243 at r3 (raw file):

    // Minimum and maximum sending precisions we allow
    if sending_precision < -(XRPL_TOKENS_DECIMALS as i32)

Why do you use the XRPL_TOKENS_DECIMALS for the validation of the min/max sending_precision ? They are independent values.

---

contract/src/contract.rs line 688 at r3 (raw file):

    let exponent = decimals as i32 - sending_precision;

    let amount_to_send = amount.checked_div(Uint128::new(10u128.pow(exponent.unsigned_abs())))?;

What if the exponent is zero here? Won't it be div by zero? Because zero exponent is a valid case as well.

---

contract/src/error.rs line 17 at r3 (raw file):

    #[error(transparent)]
    OverflowError(#[from] OverflowError),

The OverflowError and DivideByZeroError and Payment are not used in the code. Why do you need them here?

---

contract/src/state.rs line 52 at r3 (raw file):

    pub currency: String,
    pub coreum_denom: String,
    pub sending_precision: i32, // This precision will indicate the minimum we can send. Example: If sending precision = 2, The minimum we can send is 0.01

The comment is partially correct. So let's remove it, and the same for the max_holding_amount.

---

contract/src/tests.rs line 108 at r1 (raw file):

Previously, keyleu (Keyne) wrote…

You mean generate a func to generate the structure passing the values? I don't so, won't change much.

Usually when you have a constant in the code, that constant means something, so you need to match it with the logic. But when you have gen_signature_function, you understand that that any signature is needed, and you won't have to copy/paste the string and modify just one letter everywhere.

[keyleu]

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

---

contract/src/contract.rs line 50 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Don't see the changes for the amount.

Fixed it.

---

contract/src/contract.rs line 49 at r3 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Did you compute this using the spec and recommended value?

We actually agreed to put this max holding amount initially.

---

contract/src/contract.rs line 243 at r3 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Why do you use the XRPL_TOKENS_DECIMALS for the validation of the min/max sending_precision ? They are independent values.

Like we discussed, even though the max sending amount is related, I added constants for it.

---

contract/src/contract.rs line 688 at r3 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

What if the exponent is zero here? Won't it be div by zero? Because zero exponent is a valid case as well.

Any number with exponent zero is equal to 1 (except 0^0). And it's also included in the tests.

---

contract/src/error.rs line 17 at r3 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

The OverflowError and DivideByZeroError and Payment are not used in the code. Why do you need them here?

I don't explicitly sue them but they are in the code.

For example:
When I do a checked_add or a checked_mul I can get an OverflowError and I propagate it as a ContractError. Even though you don't see them, they are there when I use the operator '?'

---

contract/src/state.rs line 52 at r3 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

The comment is partially correct. So let's remove it, and the same for the max_holding_amount.

Removed.

---

contract/src/tests.rs line 108 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Usually when you have a constant in the code, that constant means something, so you need to match it with the logic. But when you have gen_signature_function, you understand that that any signature is needed, and you won't have to copy/paste the string and modify just one letter everywhere.

Sure, added one for both address and pubkeys.

[dzmitryhil]

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

---

contract/src/contract.rs line 49 at r3 (raw file):

Previously, keyleu (Keyne) wrote…

We actually agreed to put this max holding amount initially.

Can you please recompute it with the suggested formula?

---

contract/src/tests.rs line 626 at r4 (raw file):

            },
            XRPLToken {
                issuer: "rf1BiGeXwwQoi8Z2ueFYTEXSwuJYfV2Jp2".to_string(), //Valid issuer

You can replace the address constants everywhere in the code.

[keyleu]

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

---

contract/src/contract.rs line 49 at r3 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Can you please recompute it with the suggested formula?

Done!

---

contract/src/tests.rs line 626 at r4 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

You can replace the address constants everywhere in the code.

Right, forgot this one!

[dzmitryhil]

Reviewable status: 1 of 14 files reviewed, all discussions resolved (waiting on @miladz68, @wojtek-coreum, and @ysv)

---

contract/src/tests.rs line 626 at r4 (raw file):

Previously, keyleu (Keyne) wrote…

Right, forgot this one!

Don't see changes

[miladz68]

Reviewed 7 of 10 files at r3, 3 of 5 files at r4, 3 of 3 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @wojtek-coreum and @ysv)