Contract: Change relayer claiming for all to individual claiming specifying amounts

[keyleu]

Description

• Like we discussed, for Coreum tokens, relayer accounts might be frozen/whitelisted so we can't automatically claim for all relayers when one relayer claims. Thus, the claim will be done individually per relayer specifying how much of each denom he wants to claim.

• Query to check fees is changed to check per address (because each address can claim at their own pace).

• ClaimFees will provide an array of coins that the relayer wants to claim from his collected fees.

• The way Fees are collected is similar to how Refundable amounts are collected for users (Contract: Change automatic token recovery to manual claiming for rejected transactions on XRPL #75) with 1 particularity: Since every time we divide the fees for relayers there might be a remainder (e.g. 20 fee / 3 relayers = 6, remainder: 2). To avoid loosing these remainders when collecting, we keep them in a FEE_REMAINDER array so that next time fees are collected we take them into account when dividing the fees again. This way, the contract doesn't accumulate these remainder balances over time and slowly returns them to the relayers.

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 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dzmitryhil, @keyleu, @miladz68, and @wojtek-coreum)

---

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

    state::{
        Config, ContractActions, CoreumToken, TokenState, XRPLToken, AVAILABLE_TICKETS, CONFIG,
        COREUM_TOKENS, FEES_COLLECTED, FEES_REMAINDER, PENDING_OPERATIONS, PENDING_TICKET_UPDATE,

nit: FEE_REMAINDERS ?
I guess you store a vector of reminders where each one is a reminder for specific coin ?

Code quote:

FEES_REMAINDER

---

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

        let mut fees_remainder = FEES_REMAINDER.load(storage)?;
        // We add the new fees to the possible remainders that we had before and use those amounts to allocate them to relayers
        let total_fee = match fees_remainder.iter_mut().find(|c| c.denom == fee.denom) {

shouldn't fees_remainder be map instead of array ?

---

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

            Some(found_coin) => found_coin.amount -= coin.amount,
            None => {
                return Err(ContractError::RelayerFeeNotClaimable {

The issue here is that amount claimed is less than fees earned.
I suggest we name error accordingly. Because NotClaimable sounds like it is blocked or there is some other issue

Code quote:

RelayerFeeNotClaimable

[ysv]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dzmitryhil, @keyleu, @miladz68, and @wojtek-coreum)

---

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

}

pub fn check_and_update_relayer_fees(

nit: we always try to avoid having multiple actions/verbs in name
Here better name is smth like: subtract_relayer_fees or withdraw_relayer_fees

[ysv]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @dzmitryhil, @keyleu, @miladz68, and @wojtek-coreum)

---

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

    state::{
        Config, ContractActions, CoreumToken, TokenState, XRPLToken, AVAILABLE_TICKETS, CONFIG,
        COREUM_TOKENS, FEES_COLLECTED, FEES_REMAINDER, PENDING_OPERATIONS, PENDING_TICKET_UPDATE,

Why don't we use map of maps here ?

{
"address1": {"ucore": 1000, "uabc": 10000000}
"address2": {"ubbb": 1}
}

Code quote:

FEES_REMAINDER

[keyleu]

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

---

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

Previously, ysv (Yaroslav Savchuk) wrote…

nit: FEE_REMAINDERS ?
I guess you store a vector of reminders where each one is a reminder for specific coin ?

Correct, changed name.

---

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

Previously, ysv (Yaroslav Savchuk) wrote…

shouldn't fees_remainder be map instead of array ?

Both are fine. I guess a Map is more efficient and cleaner to navigate. So, I changed it to a map.

---

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

Previously, ysv (Yaroslav Savchuk) wrote…

nit: we always try to avoid having multiple actions/verbs in name
Here better name is smth like: subtract_relayer_fees or withdraw_relayer_fees

Understood. Changed it.

---

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

Previously, ysv (Yaroslav Savchuk) wrote…

The issue here is that amount claimed is less than fees earned.
I suggest we name error accordingly. Because NotClaimable sounds like it is blocked or there is some other issue

Changed it to NotEnoughFeesToClaim.

[keyleu]

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

---

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

Previously, ysv (Yaroslav Savchuk) wrote…

Why don't we use map of maps here ?

{
"address1": {"ucore": 1000, "uabc": 10000000}
"address2": {"ubbb": 1}
}

What do you mean a map of maps? The remainders can't be given out because they are too small to divide, I need to keep them in a common pool. I changed it into a map tho

[keyleu]

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

---

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

Previously, keyleu (Keyne) wrote…

What do you mean a map of maps? The remainders can't be given out because they are too small to divide, I need to keep them in a common pool. I changed it into a map tho

Also: I've tried using Map of Maps in wasmd VM and it gave weird behavior and errors in the past where I couldn't even execute the contract.

[keyleu]

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

---

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

Previously, keyleu (Keyne) wrote…

Also: I've tried using Map of Maps in wasmd VM and it gave weird behavior and errors in the past where I couldn't even execute the contract.

Oh I understand maybe why you want map of maps. I think it's cleaner this way for two reasons:

1. For the queries we are going to return a Vec of Coin so we can immediately return the state when we query the fees collectable instead of having to construct the Vec from the Map every time we query.
2. We can keep operating with Coin all the time which imo makes the code more readable.

Also I think the code will be easier to read if we don't use a Map of Maps.

[ysv]

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

---

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

We can keep operating with Coin all the time which imo makes the code more readable.

Makes sense

---

contract/src/error.rs line 170 at r2 (raw file):

    #[error(
        "NotEnoughFeesToClaim: The fee {} {} is not claimable because there is are not enough fees collected",

nit: you include fee which user claimed in error. Do we want to include balance available for claiming also ?

[keyleu]

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

---

contract/src/error.rs line 170 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

nit: you include fee which user claimed in error. Do we want to include balance available for claiming also ?

Actually I can't do it in an easy way (because users can send multiple elements of the array with same coin). But it's actually returned in the query before they claim. So it is known beforehand already.

[miladz68]

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

[miladz68]

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

---

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

                let mut fees_collected = FEES_COLLECTED
                    .may_load(storage, relayer.coreum_address.to_owned())?
                    .unwrap_or_default();

there are different reason for error, if the error is not found then you intialize, otherwise you are owerwriting the values, which is not correct.
Maybe the map should be initialized once the relayer is added, and this error is handled differently.

[keyleu]

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

---

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

Previously, miladz68 (milad) wrote…

there are different reason for error, if the error is not found then you intialize, otherwise you are owerwriting the values, which is not correct.
Maybe the map should be initialized once the relayer is added, and this error is handled differently.

There is no error. I'm using may_load which never errors. It returns fees if they are there or None if the relayer has never collected fees. I'm not overwriting any values at any time.

If I was using load then like you said would be the case yes.

[ysv]

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

[miladz68]

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

---

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

Previously, keyleu (Keyne) wrote…

There is no error. I'm using may_load which never errors. It returns fees if they are there or None if the relayer has never collected fees. I'm not overwriting any values at any time.

If I was using load then like you said would be the case yes.

deosnt' this function unwrap_or_default() return defaultt value is there is an error ?

[keyleu]

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

---

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

Previously, miladz68 (milad) wrote…

deosnt' this function unwrap_or_default() return defaultt value is there is an error ?

No.

This function does the following:

If value returned from may_load is Option<Vec> returns the vec.
If value returned from may_load is None returns an empty vec (because default value for a vec is the vec without elements).

[keyleu]

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

---

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

Previously, keyleu (Keyne) wrote…

No.

This function does the following:

If value returned from may_load is Option<Vec> returns the vec.
If value returned from may_load is None returns an empty vec (because default value for a vec is the vec without elements).

This code would be equivalent of:

let mut fees collected = match FEES_COLLECTED .may_load(storage, relayer.coreum_address.to_owned())? {
Some(vector) => vector,
None => vec![],
};

[miladz68]

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