Post-merge review for smart contract

[ysv]

Description

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 

[keyleu]

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

---

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

const XRP_SUBUNIT: &str = "drop";

const COREUM_CURRENCY_PREFIX: &str = "coreum"; // As for these prefixes shouldn't we use coreumbridge prefix for both ? But it is kinda long so maybe we can shorten it.

This is the prefix for the tokens in XRPL, so tokens in XRPL will have the structure coreum.... and in coreum we will have xrpl.... Similar how ibc uses ibc/. We discussed this with Reza and he was fine with this naming. I think coreumbridge is maybe too long.

---

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

    }

    // We want to check that amount equal to issue fee was sent. We need it to issue XRP on Coreum.

Correct.

---

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

    let xrp_issue_msg = CosmosMsg::from(CoreumMsg::AssetFT(Issue {
        symbol: XRP_SYMBOL.to_string(), // IMO we should use wrapped XRP (WXRP). As for subunit I'm not sure.

It can be changed, don't think it's a big deal as the denom will be used in most cases.

---

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

        precision: 6,
        initial_amount: Uint128::zero(),
        description: None, // I would add a small description here. WDYT ?

Yes we can add a description if you want, it doesn't affect the functionality.

---

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

}

fn update_ownership( // Only admin is allowed to do it, right ?

Only owner can do that yes, inside the update_ownership it checks that the owner is calling it.

---

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

// I can see that owner aka admin is the only one who can do this.
//  But as far as I remember Dima mentioned that relayers will be able to trigger this ?

I think the initial version will have the admin registering but the next version will have relayers triggering it but Dima can confirm.

---

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

    // We generate a currency creating a Sha256 hash of the denom, the decimals and the current time
    // I didn't get why we need block time & decimals here ? Lets discuss

Because we are only taking 10 characters of the hash, so there is a very small chance of collision, adding the block time we can make sure that in case it happens we can try again next block to register it.

---

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

    let xrpl_currency = format!("{}{}", COREUM_CURRENCY_PREFIX, hex_string);

    // this will probably never fail because we have block time inside hash.

Probably yes, but there is a small chance that it can happen because we are taking only 10 characters. It's a way to make sure that IF it happens it fails.

---

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

    deps: DepsMut<CoreumQueries>,
    env: Env,
    issuer: String, // I thought that issuer is always smart contract address. But it seems like this is a param ? Or this one is xrpl issuer ?

This is the xrpl issuer

---

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

    // We generate a random denom creating a Sha256 hash of the issuer, currency, decimals and current time
    let mut hasher = Sha256::new(); // I can see that you have a helper for this `hash_bytes`. Maybe use it here also ?

Done!

---

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

            issuer,
            currency.clone(),
            XRPL_TOKENS_DECIMALS, // So as we found out token decimals don't work in simple way in xrpl. IMO we should specify precision for each token separately, WDYT ?

I thought all tokens have 15 decimals? In any case we can modify this in the next PR when we handle the decimal problems.

---

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

    // Since symbol is just display thingy used by UI, we don't need to make it unique & use hash for it.
    // I suggest we just use currency or W+currency from XRPL so it looks better.
    // As for subunit, since it is a unique identifier, of a denom we should use hash.

But, we might have a problem if they are the same or not?

---

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

    };

    COREUM_DENOMS.save(deps.storage, denom.clone(), &Empty {})?; // we just store key and the value is Empty, right ? I guess this is done to just mark denom as reserved.

Correct

---

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

    let mut evidences: Evidences;
    match EVIDENCES.may_load(storage, evidence.get_hash())? { // EVIDENCES are evidences for pending txs right ?

Yes, once we get enough evidences we confirm the TX

[dzmitryhil]

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

---

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

Previously, keyleu (Keyne) wrote…

It can be changed, don't think it's a big deal as the denom will be used in most cases.

IMO both options are correct.

---

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

Previously, keyleu (Keyne) wrote…

Yes we can add a description if you want, it doesn't affect the functionality.

But if the description is the same it is equal to no-description IMO.

---

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

Previously, keyleu (Keyne) wrote…

I think the initial version will have the admin registering but the next version will have relayers triggering it but Dima can confirm.

Right, now just owner can call it.

---

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

Previously, keyleu (Keyne) wrote…

Because we are only taking 10 characters of the hash, so there is a very small chance of collision, adding the block time we can make sure that in case it happens we can try again next block to register it.

Right.

---

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

Previously, keyleu (Keyne) wrote…

Probably yes, but there is a small chance that it can happen because we are taking only 10 characters. It's a way to make sure that IF it happens it fails.

But better to check to be sure. :-)

---

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

Previously, keyleu (Keyne) wrote…

This is the xrpl issuer

Right, this is the XRPL issuer account address.

---

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

Previously, keyleu (Keyne) wrote…

I thought all tokens have 15 decimals? In any case we can modify this in the next PR when we handle the decimal problems.

Actually, we will have 2 precisions, token-precision (for the registration in metadata) and min-precision for the sending truncation.

---

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

Previously, keyleu (Keyne) wrote…

But, we might have a problem if they are the same or not?

Right @keyleu . Only the issuer + currency is unique.

---

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

Previously, keyleu (Keyne) wrote…

Correct

This is the way of Set handling in the wasm/rust.

---

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

Previously, keyleu (Keyne) wrote…

Yes, once we get enough evidences we confirm the TX

Evidences are not just for the pending transactions, we use them for the TX confirmation/desjection as well. This is

[dzmitryhil]

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

[keyleu]

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

---

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

}

// nit: Linter tells me "Duplicated code fragment (7 lines long) " for query_xrpl_tokens & query_coreum_tokens. not sure we can unify

Removed this query because we don't need/want it.

---

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

) -> StdResult<XRPLTokenResponse> {
    let mut key;
    if issuer.is_none() && currency.is_none() { // if neither of currency or issuer is given, we return all XRP I guess. But what if one of them is missing ?

I removed this query anyways because we don't need it.

---

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

    if query_params_res.params.issue_fee != one_coin(info)? {
        return Err(ContractError::InvalidIssueFee {}); // I disagree with error name here. Issue fee is valid but amount attached to tx is not.

I disagree with this. The price of issuing is X, if you give me more or less I consider it invalid.

---

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

}

// nit: We are planning to add feature where we can mint tokens on some dest address directly in current phase.

Added todo

---

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

    #[error("TokenNotRegistered: The token must be registered first before bridging")]
    TokenNotRegistered {}, // is it about XRPL or Coreum token.

Want to use it for both.

[ysv]

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

---

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

Previously, keyleu (Keyne) wrote…

This is the prefix for the tokens in XRPL, so tokens in XRPL will have the structure coreum.... and in coreum we will have xrpl.... Similar how ibc uses ibc/. We discussed this with Reza and he was fine with this naming. I think coreumbridge is maybe too long.

1. question to Reza do we want to use specific "coreumbridge" prefix for such assets

2. how we wnat to name XRP on coreum WXRP or XRP ?

3. do we want symbol to be hash (it should be unique)

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

Right.

Needs Reza's input

---

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

    CONFIG.save(deps.storage, &config)?;

    // I can see that we treat XRP differently from all other tokens.

agreed to change it & treat all tokens in the same way on contract side but handle logic difference on relayer

lets keep these values in constants currency: "XRP", issuer: "rrrrrrrrrrrrrrrrrrrrrho"

---

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

    let issue_msg = CosmosMsg::from(CoreumMsg::AssetFT(Issue {
        symbol: symbol_and_subunit.to_uppercase(),
        subunit: symbol_and_subunit.clone(),

Decided to Ask Reza if we want to allow to pass symbol & subunit & currency when registering token.

---

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

Previously, keyleu (Keyne) wrote…

I disagree with this. The price of issuing is X, if you give me more or less I consider it invalid.

InvalidFundsAmount

---

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

    InvalidIssueFee {},

    // nit: well that is not random.

needs discussion with Reza

---

contract/src/evidence.rs line 12 at r2 (raw file):

#[cw_serde]
pub enum Evidence {
    // nit: XRPLToCoreum -> XRPLToCoreumTransfer ?

decided: XRPLToCoreumTransfer

---

contract/src/evidence.rs line 68 at r2 (raw file):

    evidence: Evidence,
) -> Result<bool, ContractError> {
    if EXECUTED_EVIDENCE_OPERATIONS.has(storage, evidence.get_tx_hash().to_lowercase()) {

1. in msg.rs & contract.rs: accept_evidence -> send_evidence
2. here: we keep it
3. EXECUTED_EVIDENCE_OPERATIONS -> PROCESSED_TXS
4. EVIDENCES -> TX_EVIDENCES

---

contract/src/state.rs line 56 at r2 (raw file):

// 2. Also comments on top of XRPL_CURRENCIES and COREUM_DENOMS should be improved.

// 3. lets discuss where we use token, currency and denom.

3 - not relevant

---

contract/src/state.rs line 63 at r2 (raw file):

pub const XRPL_TOKENS: Map<String, XRPLToken> = Map::new(TopKey::XRPLTokens.as_str());
// XRPL-Currencies used
pub const XRPL_CURRENCIES: Map<String, Empty> = Map::new(TopKey::XRPLCurrencies.as_str());

XRPL_CURRENCIES -> USED_XRPL_CURRENCIES_FOR_COREUM_TOKENS

COREUM_DENOMS - not needed you can use COREUM_TOKENS

---

contract/src/tests.rs line 459 at r2 (raw file):

        let test_tokens = vec![
            XRPLToken {
                // shall we add at least some basic validations for issuer and currency to avoid stupid mistakes when registering ?

decided to add strict validations for issuer, currency TXID

[keyleu]

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

---

contract/src/tests.rs line 459 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

decided to add strict validations for issuer, currency TXID

what kinds of validations should we have? Can we make a list?

[dzmitryhil]

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

[ysv]

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

---

contract/src/tests.rs line 459 at r2 (raw file):

Previously, keyleu (Keyne) wrote…

what kinds of validations should we have? Can we make a list?

issuer -> xrpl address regexp
currency -> there are 2 types of currencies in xrpl: 3-chars or 40 byte hex. I would add regexp here also

Since we will manually register tokens, it is error/typo prone so I would add as strict validations as possible. As for txs submitted by relayer - simple validations should be enough since it is automated and tested

[keyleu]

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

---

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

Previously, ysv (Yaroslav Savchuk) wrote…

agreed to change it & treat all tokens in the same way on contract side but handle logic difference on relayer

lets keep these values in constants currency: "XRP", issuer: "rrrrrrrrrrrrrrrrrrrrrho"

This is changed.

---

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

Previously, ysv (Yaroslav Savchuk) wrote…

InvalidFundsAmount

Changed.

---

contract/src/evidence.rs line 12 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

decided: XRPLToCoreumTransfer

Changed

---

contract/src/evidence.rs line 68 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

1. in msg.rs & contract.rs: accept_evidence -> send_evidence
2. here: we keep it
3. EXECUTED_EVIDENCE_OPERATIONS -> PROCESSED_TXS
4. EVIDENCES -> TX_EVIDENCES

Changed

---

contract/src/state.rs line 56 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

3 - not relevant

1 and 2 are done.

---

contract/src/state.rs line 63 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

XRPL_CURRENCIES -> USED_XRPL_CURRENCIES_FOR_COREUM_TOKENS

COREUM_DENOMS - not needed you can use COREUM_TOKENS

Changed it.

---

contract/src/tests.rs line 459 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

issuer -> xrpl address regexp
currency -> there are 2 types of currencies in xrpl: 3-chars or 40 byte hex. I would add regexp here also

Since we will manually register tokens, it is error/typo prone so I would add as strict validations as possible. As for txs submitted by relayer - simple validations should be enough since it is automated and tested

Added some validations. I initially used RegExp but they take too much space in the code. The wasm went from 300 Kb to 1.4MB with 2 simple RegExp matchings and the .wasm code can't go over 800KB. Therefore I added the validations without RegExp. If you think they are not enough let me know and I can make it stricter.

[keyleu]

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

---

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

Previously, ysv (Yaroslav Savchuk) wrote…

1. question to Reza do we want to use specific "coreumbridge" prefix for such assets

2. how we wnat to name XRP on coreum WXRP or XRP ?

3. do we want symbol to be hash (it should be unique)

1. Stick to coreum and xrpl.

2. Keep XRP.

3. Keep Hash.

---

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

Previously, ysv (Yaroslav Savchuk) wrote…

Decided to Ask Reza if we want to allow to pass symbol & subunit & currency when registering token.

Agreed to keep like it is (hash)