Contract: Change automatic token recovery to manual claiming for rejected transactions on XRPL

[keyleu]

Description

• Like we discussed, due to the ability of Coreum tokens to be frozen / whitelisted for addresses, we can't do automatic recovery, therefore we changed the automatic sending back when a transaction is rejected to a manual claim by the user.
• How it works: every time a transaction is rejected, instead of doing a bank send, the refundable tokens will be stored and accumulated for each user in the state of the contract (similar to how relayer fees are stored). The user can query any time how many tokens does he have pending to claim back and he can claim them back directly interacting with the contract. Obviously, if he asks for tokens that are not refundable or he asks for higher amounts that what is refundable, it will fail.

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

---

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

        }
        ExecuteMsg::ClaimFees {} => claim_fees(deps.into_empty(), info.sender),
        ExecuteMsg::ClaimRefundableAmounts { amounts } => {

IMO correct wording here & in other places is ClaimRefund(s)

---

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

    deps: DepsMut,
    sender: Addr,
    amounts: Vec<Coin>,

I disagree with the approach you took here.
In real world examples when you claim some refund you don't just send amount. Instead you specify some kind of order identifier you want to claim.

It should work the same way here. We should have some kind of identifier for transfer (txid/iternal id or smth else) & user should pass it when claiming.

So we should operate with failed transfers but not amounts everywhere. This should also reflect var names

---

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

fn query_refundable_amounts(deps: Deps, address: Addr) -> StdResult<RefundableAmountsResponse> {
    let refundable_amounts = REFUNDABLE_AMOUNTS.load(deps.storage, address)?;

I think name should be PENDING_REFUNDS

Code quote:

REFUNDABLE_AMOUNTS

---

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

        .iter_mut()
        .find(|c| c.denom == coin.denom)
    {

Why can't we have map inside map ?
Or this is painful to operate with in rust ?

Code quote:

        .find(|c| c.denom == coin.denom)
    {

[keyleu]

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

---

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

Previously, ysv (Yaroslav Savchuk) wrote…

IMO correct wording here & in other places is ClaimRefund(s)

Changed it.

---

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

Previously, ysv (Yaroslav Savchuk) wrote…

I disagree with the approach you took here.
In real world examples when you claim some refund you don't just send amount. Instead you specify some kind of order identifier you want to claim.

It should work the same way here. We should have some kind of identifier for transfer (txid/iternal id or smth else) & user should pass it when claiming.

So we should operate with failed transfers but not amounts everywhere. This should also reflect var names

How? We might not have a Transaction ID or Internal ID we can use. If transaction was invalid there is no TX hash and the ticket is refunded so it can be used again so we can't use that either. I don't have any ID I can use for those cases.

---

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

Previously, ysv (Yaroslav Savchuk) wrote…

I think name should be PENDING_REFUNDS

Changed.

---

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

Previously, ysv (Yaroslav Savchuk) wrote…

Why can't we have map inside map ?
Or this is painful to operate with in rust ?

I think it was answered in the other PR. Anyways this is removed because I changed it into Refund a TX instead of amounts.

[keyleu]

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

---

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

Previously, keyleu (Keyne) wrote…

I think it was answered in the other PR. Anyways this is removed because I changed it into Refund a TX instead of amounts.

Nevermind above comment, need to discuss.

[keyleu]

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

---

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

Previously, keyleu (Keyne) wrote…

How? We might not have a Transaction ID or Internal ID we can use. If transaction was invalid there is no TX hash and the ticket is refunded so it can be used again so we can't use that either. I don't have any ID I can use for those cases.

Like discussed, we are going to use an ID in pending operations using timestamp and ticket to claim it back.

[keyleu]

As discussed with @ysv in a call today. It was not appropriate to claim a refundable failed transaction specifying amounts as it's not how it works in the real world. Therefore, we are assigning a unique ID to each pending operation made by "block_timestamp"-"ticket/sequence number" which we know for a fact that it's unique because a ticket can not be used at the same time in the same block.

The reason why we can't simply use the XRPL tx_hash is because invalid transactions don't have a hash, so we needed something different.
This also provides our pending transactions with a unique ID which may be useful to visualize later on.

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

[keyleu]

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

---

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

Previously, keyleu (Keyne) wrote…

Like discussed, we are going to use an ID in pending operations using timestamp and ticket to claim it back.

Changed.

[miladz68]

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

[ysv]

Reviewed 2 of 7 files at r3, 3 of 6 files at r4, 3 of 3 files at r5, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @dzmitryhil, @keyleu, and @wojtek-coreum)

---

contract/src/contract.rs line 212 at r5 (raw file):

        ExecuteMsg::SaveEvidence { evidence } => save_evidence(
            deps.into_empty(),
            env.block.time.seconds(),

I think we have to discuss this on call because I didn't get the implementation fully.
I thought that we will add this ID for ExecuteMsg::SendToXRPL case but not everywhere. Maybe inside CoreumToXRPLTransfer

I guess there is a reason you added env.block.time.seconds() everywhere but lets discuss because currently I don't have clear understanding.

---

contract/src/contract.rs line 257 at r5 (raw file):

            pending_operation_id,
        } => claim_pending_refund(deps.into_empty(), info.sender, pending_operation_id),
        ExecuteMsg::ClaimFees { amounts } => claim_fees(deps.into_empty(), info.sender, amounts),

nit: I would name it ClaimRelayerFees everywhere if you prefer

Code quote:

ExecuteMsg::ClaimFees

---

contract/src/msg.rs line 83 at r5 (raw file):

    },
    // Claim refunds. User who can claim amounts due to failed transactions can do it with this message.
    ClaimRefunds {

if you support a single operation id then it should be singular ClaimRefund

Code quote:

ClaimRefunds

---

contract/src/operation.rs line 17 at r5 (raw file):

#[cw_serde]
pub struct Operation {
    pub id: String, // We will use this unique id (block timestamp - operation_id) for users to claim their funds back per operation id instead of with amounts

nit: I'm not sure instead of with amounts is needed here in this description

Code quote:

instead of with amounts

---

contract/src/operation.rs line 17 at r5 (raw file):

#[cw_serde]
pub struct Operation {
    pub id: String, // We will use this unique id (block timestamp - operation_id) for users to claim their funds back per operation id instead of with amounts

but this applies only to specific type of operations because not everything is XRPLToCoreum transfer here, correct ?

This comment is more relevant to id inside PENDING_REFUNDS

---

contract/src/operation.rs line 219 at r5 (raw file):

    storage: &mut dyn Storage,
    sender: Addr,
    pending_operation_id: String,

nit: pending_refund_id ?

Code quote:

pending_operation_id

---

contract/src/state.rs line 25 at r5 (raw file):

    FeesCollected = b'b',
    PendingRefunds = b'c',
    FeeRemainders = b'd',

nit: IMO it is better to keep related things nearby so Fees are defined one after another and then PendingRefunds

Just swap PendingRefunds & FeeRemainders

[keyleu]

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

---

contract/src/contract.rs line 212 at r5 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

I think we have to discuss this on call because I didn't get the implementation fully.
I thought that we will add this ID for ExecuteMsg::SendToXRPL case but not everywhere. Maybe inside CoreumToXRPLTransfer

I guess there is a reason you added env.block.time.seconds() everywhere but lets discuss because currently I don't have clear understanding.

Sure let's discuss it. The idea was to add a unique ID to pending operation so that transactions from SendToXRPL that failed were refunded.

But Pending Operations are created in more places than SendToXRPL so I added this ID to ALL pending operations (because I don't think an ID should be optional).

Basically everywhere we are creating a Pending Operation I give it the unique ID (timestamp - operationid) that we discussed.

---

contract/src/contract.rs line 257 at r5 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

nit: I would name it ClaimRelayerFees everywhere if you prefer

Renamed.

---

contract/src/msg.rs line 83 at r5 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

if you support a single operation id then it should be singular ClaimRefund

Changed.

---

contract/src/operation.rs line 17 at r5 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

nit: I'm not sure instead of with amounts is needed here in this description

Removed.

---

contract/src/operation.rs line 17 at r5 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

but this applies only to specific type of operations because not everything is XRPLToCoreum transfer here, correct ?

This comment is more relevant to id inside PENDING_REFUNDS

Moved it to PendingRefund

---

contract/src/operation.rs line 219 at r5 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

nit: pending_refund_id ?

Changed

---

contract/src/state.rs line 25 at r5 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

nit: IMO it is better to keep related things nearby so Fees are defined one after another and then PendingRefunds

Just swap PendingRefunds & FeeRemainders

Swapped them.

[ysv]

Reviewed 4 of 7 files at r7, 3 of 3 files at r8, 3 of 3 files at r10, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dzmitryhil, @miladz68, and @wojtek-coreum)