send xrpl to coreum

[keyleu]

Description

Contract flow for sending XRPL tokens to Coreum. Relayers will relay transactinos and we will get evidences until threshold is reached, then we mint tokens and send to receiver.

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]

Update the PR description please and update Authors checklist with the check-marks. Same for future PRs.

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

---

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

        }
        ExecuteMsg::SendFromXRPLToCoreum {
            hash,

I would call it tx_hash.

---

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

}

fn send_from_xrpl_to_coreum(

How about calling that function accept_evidence and using an enum instead? This might help to check the common things in one place for all evidences.

---

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

    let to_hash = format!(
        "{}{}{}{}{}{}",
        hash,

Let's lowercase the tx_hash, to prevent potential double processing,

---

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

        amount,
        recipient,
        Operation::XRPLToCoreum.as_str()

Don't you want to create a new typeSendFromCoreumToXRPLEvidence which you can use?
In that case, that new type might implement the Trait Evidence, which has 3 function BuildHash, GetOperation and GetTxHash. The BuildHash takes the GetOperation + args and builds the hash. That will help you not to forget to add the operation to the final hash.
The function GetTxHash should be used to prevent double execution of the same TX twice (you haven't implemented that logic here yet bu we should since because of the rounding rules we might change the amount, in the future version amd re-process the transaction).

So that case, the function evidences_handler accepts the Evidence trait instead of a string.
Also, it would be nice to move, that code to the evidence file.

---

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

    Ok(response
        .add_attribute("action", ContractActions::SendFromXRPLToCoreum.as_str())

And please threshold_reached to the response attributes as well.

---

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

}

fn hasher(bytes: Vec<u8>) -> String {

I would propose to call it hash_bytes or any other verb.

---

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

}

fn evidences_handler(

I would propose to call it handle_evidence

---

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

            EXECUTED_OPERATIONS.save(deps.storage, evidence.clone(), &Empty {})?;
            EVIDENCES.remove(deps.storage, evidence.clone());
            //We send the tokens to the recipient

That is not true. Now the function is more abstract.

---

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

        //We have enough evidences, we can execute the operation
        EXECUTED_OPERATIONS.save(deps.storage, evidence.clone(), &Empty {})?;
        //We send the tokens to the recipient

Check please all comments in the function.

---

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

        //Register a token
        for token in test_tokens.clone() {

Why do you need the Vec and loop if you use just one token?

---

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

                    currency: token.currency,
                },
                &coins(10_000_000, FEE_DENOM),

For simplicity you can add a helper func to query it from the asset ft params, we do it that way in go tests.

[dzmitryhil]

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

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

Let's lowercase the tx_hash, to prevent potential double processing,

Actually, we don't need to do it in case we do it for the verification on the processed XRPL TXs. (Wrote about it in the flowing comment)

[dzmitryhil]

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

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

Don't you want to create a new typeSendFromCoreumToXRPLEvidence which you can use?
In that case, that new type might implement the Trait Evidence, which has 3 function BuildHash, GetOperation and GetTxHash. The BuildHash takes the GetOperation + args and builds the hash. That will help you not to forget to add the operation to the final hash.
The function GetTxHash should be used to prevent double execution of the same TX twice (you haven't implemented that logic here yet bu we should since because of the rounding rules we might change the amount, in the future version amd re-process the transaction).

So that case, the function evidences_handler accepts the Evidence trait instead of a string.
Also, it would be nice to move, that code to the evidence file.

Also I would be nice to have a Validate method for the Evidence to do the basic validation. For example, to check that hash is not empty. The recipient is valid and etc.

[miladz68]

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @keyleu, @wojtek-coreum, and @ysv)

---

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

    }

    // We generate a random currency creating a Sha256 hash of the denom, the decimals and the current time

the currency that we create is not really random.

---

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

    deps: DepsMut,
    sender: Addr,
    hash: String,

what is this hash, the name should provide more information on what this hash represents !

---

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

        }

        if evidences.relayers.len() + 1 == config.evidence_threshold as usize {

this if statment is a little confusing for me, why we check that evidence+1 is greater than threshold ? it should be simpler that you try to the to the evidence list if the relayer is not already present, and check if the threshold is reached.

[keyleu]

Ok, done.

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

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

I would call it tx_hash.

Changed it.

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

And please threshold_reached to the response attributes as well.

added

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

I would propose to call it hash_bytes or any other verb.

Ok that sounds fine.

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

I would propose to call it handle_evidence

Fixed

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

That is not true. Now the function is more abstract.

Correct, fixed.

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

Check please all comments in the function.

Fixed these comments.

[keyleu]

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

---

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

Previously, miladz68 (milad) wrote…

the currency that we create is not really random.

Right, removed the random part as it's misleading.

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

How about calling that function accept_evidence and using an enum instead? This might help to check the common things in one place for all evidences.

Refactored this.

---

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

Previously, miladz68 (milad) wrote…

what is this hash, the name should provide more information on what this hash represents !

I changed this to tx_hash as suggested by Dima. It's the transaction hash in the XRPL.

---

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

Previously, miladz68 (milad) wrote…

this if statment is a little confusing for me, why we check that evidence+1 is greater than threshold ? it should be simpler that you try to the to the evidence list if the relayer is not already present, and check if the threshold is reached.

That's what I'm doing. I'm checking that the relayer hasn't provided an evidence yet (Error above) and if he hasn't, I check if the threshold is reached then we can execute the operation.

[keyleu]

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

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

Why do you need the Vec and loop if you use just one token?

just so that in the future we want to test with more tokens or extend the tests it's easier to add them.

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

For simplicity you can add a helper func to query it from the asset ft params, we do it that way in go tests.

done

[keyleu]

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

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

Actually, we don't need to do it in case we do it for the verification on the processed XRPL TXs. (Wrote about it in the flowing comment)

Acknowledged.

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

Also I would be nice to have a Validate method for the Evidence to do the basic validation. For example, to check that hash is not empty. The recipient is valid and etc.

Added evidence validation. For now only validating amount not 0. We can discuss what other validations we can add.

[dzmitryhil]

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

---

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

Previously, keyleu (Keyne) wrote…

added

In that version of the contract, you don't return the attributes in case the threshold_reached is false, but should.

---

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

    let denom;

    match evidence.clone() {

You can check this in the single match after the check_evidence as well as if threshold_reached

---

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

            //Create issuer+currency key to find denom on coreum.
            let mut key = issuer.clone();
            key.push_str(currency.as_str());

How about creating a function for it and re-using it here and in the registry? And in-general it would be nice to have functions for keys in case the key is compound.

---

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

    #[error("UnauthorizedOperation: Sender is not a valid relayer")]
    UnauthorizedOperation {},

You use the UnauthorizedOperation as a type, but the message is about the sender. Probably better call it UnauthorizedSender ?

---

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

    #[error("TokenNotRegistered: This token must be registered first before bridging")]
    TokenNotRegistered {},

Will you reuse that error for the coreum tokens?

---

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

    #[error("OperationAlreadyExecuted: This operation has already been executed, no need to keep relaying")]
    OperationAlreadyExecuted {},

no need to keep relaying that part can be removed IMO

---

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

    #[error(
        "EvidenceAlreadyProvided: This relayer already provided its evidence for this operation"

How about not to use the This in the error messages. And its as well.
For example:
The relayer already provided the evidence for the operation.
It sounds more natural. (Same for all messages).

---

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

}

impl Evidence {

What about the basic validation of the Evidence ?

---

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

    pub fn get_tx_hash(&self) -> String {
        match self {
            Evidence::XRPLToCoreum { tx_hash, .. } => tx_hash.clone().to_lowercase(),

Once you update the handle_evidence to use the type instead of two strings, move the to_lowercase() to the handle evidence in order not to duplicate it for future evidences.

---

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

}

pub fn handle_evidence(deps: DepsMut, sender: Addr, evidence: String, tx_hash: String) -> Result<bool, ContractError> {

The func should accept the Evidence type, not the string for the evidence: String, tx_hash: String.

---

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

    let mut threshold_reached = false;

    if EXECUTED_OPERATIONS.has(deps.storage, evidence.clone()) {

Looks like a mistake, you save the processed using tx_hash, but check that using the evidence_hash. The new test should show that failure.

---

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

    let mut threshold_reached = false;

    if EXECUTED_OPERATIONS.has(deps.storage, evidence.clone()) {

Let's call it EXECUTED_EVIDENCE_OPERATIONS

---

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

            EXECUTED_OPERATIONS.save(deps.storage, evidence.clone(), &tx_hash)?;
            EVIDENCES.remove(deps.storage, evidence.clone());
            //Threshold is reached

This comment isn't needed it's clear without it as well :-) Same for the same comment in that func.

---

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

pub enum TopKey {
    Config = b'c',
    Evidences = b'E',

How about use 1,2,3,4,5 and etc for all keys?

---

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

// Coreum denoms used
pub const COREUM_DENOMS: Map<String, Empty> = Map::new(TopKey::CoreumDenoms.as_str());
// Evidences, when enough evidences are collected, hashes are moved to completed operations

completed operations - executed operations? Same for the comment of the EXECUTED_OPERATIONS

---

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

// Evidences, when enough evidences are collected, hashes are moved to completed operations
pub const EVIDENCES: Map<String, Evidences> = Map::new(TopKey::Evidences.as_str());
// Completed operations so that we don't process the same operation twice

Based on the new logic it is a bit different.

---

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

Previously, keyleu (Keyne) wrote…

just so that in the future we want to test with more tokens or extend the tests it's easier to add them.

We need to either add them now, or use one token.

---

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

                .to_string()
                .as_str()
        ));

How about the test-case of the re-processing of the same tx hash with the different data?

[keyleu]

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

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

You use the UnauthorizedOperation as a type, but the message is about the sender. Probably better call it UnauthorizedSender ?

Hmm, okay, makes more sense.

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

Will you reuse that error for the coreum tokens?

Yes we can reuse this error now that you mention it.

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

no need to keep relaying that part can be removed IMO

Removed.

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

How about not to use the This in the error messages. And its as well.
For example:
The relayer already provided the evidence for the operation.
It sounds more natural. (Same for all messages).

OK, changed.

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

What about the basic validation of the Evidence ?

It's the validate function.

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

Once you update the handle_evidence to use the type instead of two strings, move the to_lowercase() to the handle evidence in order not to duplicate it for future evidences.

Done

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

The func should accept the Evidence type, not the string for the evidence: String, tx_hash: String.

Fixed.

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

Looks like a mistake, you save the processed using tx_hash, but check that using the evidence_hash. The new test should show that failure.

Hmmm. No, I'm saving the evidence_hash as the key of the mapping and the result of mapping is tx_hash (so that we can recover it), this is working correctly, maybe we are having a misunderstanding, discuss it?

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

Let's call it EXECUTED_EVIDENCE_OPERATIONS

Changed.

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

This comment isn't needed it's clear without it as well :-) Same for the same comment in that func.

true, removed it.

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

How about use 1,2,3,4,5 and etc for all keys?

Sure no problem, this has no effect at all for us by the way. It's just to optimize storage access having the smallest key value possible (1 byte). Changed it to numbers and if we exceed we can start using lowercase letters.

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

completed operations - executed operations? Same for the comment of the EXECUTED_OPERATIONS

Changed

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

Based on the new logic it is a bit different.

Changed the comments.

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

How about the test-case of the re-processing of the same tx hash with the different data?

At the moment this is not failing, it generates a complete different key in the EXECUTED_EVIDENCE_OPERATIONS. Should it fail? In that case we probably need an extra mapping.

[keyleu]

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

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

In that version of the contract, you don't return the attributes in case the threshold_reached is false, but should.

Yes, realized when I was fixing the previous point, putting it together. It does now.

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

You can check this in the single match after the check_evidence as well as if threshold_reached

True, code much cleaner now.

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

How about creating a function for it and re-using it here and in the registry? And in-general it would be nice to have functions for keys in case the key is compound.

Done.

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

We need to either add them now, or use one token.

Removed the array then and have only one token.

[dzmitryhil]

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

---

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

    //There are already evidences from previous relayers
    if let Some(mut evidences) = evidences {

You can simplify the function like this:

if EXECUTED\_OPERATIONS.has(deps.storage, tx\_hash.clone()) {  
    return Err(ContractError::OperationAlreadyExecuted {});  
}  
  
let mut evidences: Evidences;  
match EVIDENCES.may\_load(deps.storage, evidence\_hash)? {  
    None \=> {  
        evidences \= Evidences {  
            relayers: vec!\[sender.clone()\],  
        };  
    }  
    Some(stored\_evidences) => {  
        if stored\_evidences.relayers.contains(&sender) {  
            return Err(ContractError::EvidenceAlreadyProvided {});  
        }  
        evidences \= stored\_evidences;  
        evidences.relayers.push(sender.clone())  
    }  
}  
  
let config \= CONFIG.load(deps.storage)?;  
if evidences.relayers.len() >= config.evidence\_threshold as usize {  
    EXECUTED\_OPERATIONS.save(deps.storage, tx\_hash, evidence\_hash)?;  
    // if there is just one relayer there is nothing to delete  
    if evidences.relayers.len() != 1 {  
        EVIDENCES.remove(deps.storage, evidence\_hash.clone());  
    }  
    return Ok(true)  
} else {  
    EVIDENCES.save(deps.storage, evidence\_hash.clone(), &evidences)?;  
}  
  
return Ok(false);

Pay attention, please, that code is draft, so some logic might be broken.

[dzmitryhil]

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

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

You can simplify the function like this:

if EXECUTED\_OPERATIONS.has(deps.storage, tx\_hash.clone()) {  
    return Err(ContractError::OperationAlreadyExecuted {});  
}  
  
let mut evidences: Evidences;  
match EVIDENCES.may\_load(deps.storage, evidence\_hash)? {  
    None \=> {  
        evidences \= Evidences {  
            relayers: vec!\[sender.clone()\],  
        };  
    }  
    Some(stored\_evidences) => {  
        if stored\_evidences.relayers.contains(&sender) {  
            return Err(ContractError::EvidenceAlreadyProvided {});  
        }  
        evidences \= stored\_evidences;  
        evidences.relayers.push(sender.clone())  
    }  
}  
  
let config \= CONFIG.load(deps.storage)?;  
if evidences.relayers.len() >= config.evidence\_threshold as usize {  
    EXECUTED\_OPERATIONS.save(deps.storage, tx\_hash, evidence\_hash)?;  
    // if there is just one relayer there is nothing to delete  
    if evidences.relayers.len() != 1 {  
        EVIDENCES.remove(deps.storage, evidence\_hash.clone());  
    }  
    return Ok(true)  
} else {  
    EVIDENCES.save(deps.storage, evidence\_hash.clone(), &evidences)?;  
}  
  
return Ok(false);

Pay attention, please, that code is draft, so some logic might be broken.

For some reason, the Reviewable broke the rust code from the previous comment. :-)

[keyleu]

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

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

For some reason, the Reviewable broke the rust code from the previous comment. :-)

changed it. Made some slight changes but I guess it's simpler this way.

[dzmitryhil]

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

---

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

Previously, keyleu (Keyne) wrote…

Done.

But the name is bad :-) get_key what key?
I would call it, BuildXRPLTokensKey and move to the store.rs and actually all functions creating the key there as well (in the future).

---

contract/src/contract.rs line 265 at r6 (raw file):

    }

    let denom;

Why do you need the let denom; here?

---

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

Previously, keyleu (Keyne) wrote…

Hmmm. No, I'm saving the evidence_hash as the key of the mapping and the result of mapping is tx_hash (so that we can recover it), this is working correctly, maybe we are having a misunderstanding, discuss it?

The key for the EXECUTED_EVIDENCE_OPERATIONS should be the tx_hash, but for the EVIDENCES evidence_hash.

---

contract/src/evidence.rs line 73 at r6 (raw file):

    sender: Addr,
    evidence: Evidence,
) -> Result<bool, ContractError> {

You still need to init the tx_hash as let tx_hash = evidence.get_tx_hash().to_lowercase(); and use it here.

---

contract/src/evidence.rs line 96 at r6 (raw file):

    let config = CONFIG.load(deps.storage)?;
    if evidences.relayers.len() >= config.evidence_threshold.try_into().unwrap() {
        EXECUTED_EVIDENCE_OPERATIONS.save(

Why do you need the value for the EXECUTED_EVIDENCE_OPERATIONS, the key is enough, right? So we can use the Map<Addr, Empty> as type for it.

---

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

Previously, keyleu (Keyne) wrote…

Changed

Actually, now this is not correct :-)
Since the key here is hash of the evidence, but in the executed is tx_hash.

---

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

Previously, keyleu (Keyne) wrote…

At the moment this is not failing, it generates a complete different key in the EXECUTED_EVIDENCE_OPERATIONS. Should it fail? In that case we probably need an extra mapping.

We need to update it and have such a test.

[keyleu]

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

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

But the name is bad :-) get_key what key?
I would call it, BuildXRPLTokensKey and move to the store.rs and actually all functions creating the key there as well (in the future).

OK, I don't think we will have more of these functions tho but having it in state.rs is fine.

---

contract/src/contract.rs line 265 at r6 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Why do you need the let denom; here?

true, it was from before when it was split. It's inside now.

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

The key for the EXECUTED_EVIDENCE_OPERATIONS should be the tx_hash, but for the EVIDENCES evidence_hash.

Yes, like we discussed, this is working like this now.

---

contract/src/evidence.rs line 73 at r6 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

You still need to init the tx_hash as let tx_hash = evidence.get_tx_hash().to_lowercase(); and use it here.

saved it as lowercase directly inside the state.

---

contract/src/evidence.rs line 96 at r6 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Why do you need the value for the EXECUTED_EVIDENCE_OPERATIONS, the key is enough, right? So we can use the Map<Addr, Empty> as type for it.

Yes, this was before because I misunderstood the EXECUTED_EVIDENCE, I thought we needed a map between operation hash and tx hash but we didn't. Changed it back to Empty.

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

Actually, now this is not correct :-)
Since the key here is hash of the evidence, but in the executed is tx_hash.

Updated with accurate comments.

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

We need to update it and have such a test.

This test does this. I'm sending a different data (in this case a new_amount) with a TX_HASH that has been processed already and it correctly fails.

[dzmitryhil]

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

---

contract/src/contract.rs line 281 at r7 (raw file):

                .load(deps.storage, key)
                .map_err(|_| ContractError::TokenNotRegistered {})?;
            let threshold_reached = handle_evidence(deps, sender, evidence.clone())?;

You can move that part just after the
if !config.relayers.contains(&sender) { return Err(ContractError::UnauthorizedSender {}); }
Since it is common for all evidences.

---

contract/src/evidence.rs line 106 at r7 (raw file):

        }
        return Ok(true);
    } else {

Since you call return in the if you don't need the else and can call EVIDENCES.save just after the if

    } 
    EVIDENCES.save(deps.storage, evidence.get_hash(), &evidences)?;
    Ok(false)

[dzmitryhil]

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

---

contract/src/evidence.rs line 38 at r7 (raw file):

                    amount,
                    recipient,
                    Operation::XRPLToCoreum.as_str()

Now I see, that we can remove the Operation from the hash, and type in general, since the key for the executed operations is tx hash, so it is impossible to have the same data in the evidence for different types.
Sorry for the previous confusion.

[miladz68]

Reviewed 2 of 6 files at r2, 1 of 5 files at r5, 1 of 3 files at r6, 3 of 3 files at r7, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @keyleu, @wojtek-coreum, and @ysv)

---

contract/src/evidence.rs line 103 at r7 (raw file):

        // if there is just one relayer there is nothing to delete
        if evidences.relayers.len() != 1 {
            EVIDENCES.remove(deps.storage, evidence.get_hash());

I guess we are removing from one storage and write to another storage because we want to be able to search by tx_hash for historical transactions and by our own evidence hash if we are still processing it ?
If my assumption is correct, then we should use multi index maps, so we don't have delete and write agian, it will be cheaper and also simpler. it is also good to search by tx_hash and understand weather we are processing it or it is processed.
I will mark this as unblocking, I just wanted to discuss it.

---

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

    RegisterCoreumToken { denom: String, decimals: u32 },
    RegisterXRPLToken { issuer: String, currency: String },
    AcceptEvidence { evidence: Evidence },

I know that Dima wanted this change, but the action is meant to transfer funds from xrpl to coreum, surely we accept evidence for it, but we also accept evidence for coreum to xrpl. so what is the difference between the 2. In other words how do we distinguish between the 2 actions ?

[keyleu]

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

---

contract/src/contract.rs line 281 at r7 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

You can move that part just after the
if !config.relayers.contains(&sender) { return Err(ContractError::UnauthorizedSender {}); }
Since it is common for all evidences.

Ok done.

---

contract/src/evidence.rs line 38 at r7 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Now I see, that we can remove the Operation from the hash, and type in general, since the key for the executed operations is tx hash, so it is impossible to have the same data in the evidence for different types.
Sorry for the previous confusion.

Yes, this was added because of my misunderstanding in the past. Removed it.

---

contract/src/evidence.rs line 103 at r7 (raw file):

Previously, miladz68 (milad) wrote…

I guess we are removing from one storage and write to another storage because we want to be able to search by tx_hash for historical transactions and by our own evidence hash if we are still processing it ?
If my assumption is correct, then we should use multi index maps, so we don't have delete and write agian, it will be cheaper and also simpler. it is also good to search by tx_hash and understand weather we are processing it or it is processed.
I will mark this as unblocking, I just wanted to discuss it.

Yes, your assumption is correct. But, isn't the point of using multi index maps to index the same information with multiple indexes? In this case we are having a mapping for evidences (evidence_hash -> evidence) and one just to store the tx_hash but without mapping it to any value.

We'd still need to delete and write because we don't want the state of the contract grow. We can discuss it.

---

contract/src/evidence.rs line 106 at r7 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Since you call return in the if you don't need the else and can call EVIDENCES.save just after the if

    } 
    EVIDENCES.save(deps.storage, evidence.get_hash(), &evidences)?;
    Ok(false)

done

---

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

Previously, miladz68 (milad) wrote…

I know that Dima wanted this change, but the action is meant to transfer funds from xrpl to coreum, surely we accept evidence for it, but we also accept evidence for coreum to xrpl. so what is the difference between the 2. In other words how do we distinguish between the 2 actions ?

With matches. When receive a different type of evidence, for example coreum to xrpl, we will first define it in the Evidence type and implement all its traits.

Inside the accept_evidence function we have a match to see what kind of evidence we are getting and what actions we have to take after handling it.

[dzmitryhil]

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

[dzmitryhil]

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

[miladz68]

Reviewed 3 of 3 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @keyleu, @wojtek-coreum, and @ysv)

---

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

Previously, keyleu (Keyne) wrote…

With matches. When receive a different type of evidence, for example coreum to xrpl, we will first define it in the Evidence type and implement all its traits.

Inside the accept_evidence function we have a match to see what kind of evidence we are getting and what actions we have to take after handling it.

But we don't have evidence type as of now, do we ? I think it is a good idea to unify handle evidence function, but the smart contract entry point should be different since the functionality is different.

[miladz68]

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