Contract: Token status update implementation

[keyleu]

Description

• Allow admin to disable/enable tokens (both Coreum or XRPL) to block them from being bridged / or bridged back

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, 2 unresolved discussions (waiting on @keyleu, @miladz68, @wojtek-coreum, and @ysv)

---

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

            send_to_xrpl(deps.into_empty(), env, info, recipient)
        }
        ExecuteMsg::UpdateXRPLToken {

Why don't we have a way to update the Courem token?

---

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

    issuer: String,
    currency: String,
    status: Option<TokenState>,

Add please somewhere the comment that attributes to update are optional.

[keyleu]

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

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

Why don't we have a way to update the Courem token?

It's under it, we have UpdateCoreumToken too.

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

Add please somewhere the comment that attributes to update are optional.

Ok, added a comment in the message definition.

[dzmitryhil]

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

[ysv]

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

---

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

        .map_err(|_| ContractError::TokenNotRegistered {})?;

    update_token_state(&mut token.state, state)?;

does it always update state even if it is already in target state ?

---

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

    #[error("InvalidTokenStateUpdate: A token state can only be updated to enabled or disabled")]
    InvalidTokenStateUpdate {},
    

extra tab

---

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

All fields that can be updatable for XRPL originated tokens will be updated with this message

except issuer & currency right ?

---

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

        .unwrap();

        // If we send second evidence it should fail because token is disabled

where is this logic implemented I wasn't able to allocated it ?

what about a test where users tries to send token which is disabled ?

---

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

// Helper function to update the status of a token
pub fn update_token_state(

nit: when I see update_token_state I expect method to also set new value in store. But since it just sets attribute value here maybe better name is smth like set_token_state ?

---

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

    if let Some(new_state) = new_state {
        if (*state).eq(&TokenState::Inactive) || (*state).eq(&TokenState::Processing) {
            return Err(ContractError::TokenStateNotUpdatable {});

nit: better error name: TokenStateIsImmutable "current token state is immutable"

---

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

        }
        if new_state.eq(&TokenState::Inactive) || new_state.eq(&TokenState::Processing) {
            return Err(ContractError::InvalidTokenStateUpdate {});

nit better error name: InvalidTargetTokenState

you can also rename local variable to target_state if u like

[ysv]

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

---

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

Previously, ysv (Yaroslav Savchuk) wrote…

where is this logic implemented I wasn't able to allocated it ?

what about a test where users tries to send token which is disabled ?

Also I'm not sure how disabled state should work for relayers. Maybe it should affect only users but if transfer happened relayers should be able to provide evidences ? Shall we discuss this ?

[keyleu]

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

---

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

Previously, ysv (Yaroslav Savchuk) wrote…

does it always update state even if it is already in target state ?

Inside this function it checks that you are not updating to an invalid state. If you are sending Disabled or Enabled it will update it (if it's already enabled for example nothing will change).

---

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

Previously, ysv (Yaroslav Savchuk) wrote…

extra tab

Done.

---

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

Previously, ysv (Yaroslav Savchuk) wrote…

All fields that can be updatable for XRPL originated tokens will be updated with this message

except issuer & currency right ?

What I meant by this comment is that all fields that can (for now we only have state so it's the only one) will be added here. The idea is that this message will be extended with the future fields that we will allow to update.

---

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

Previously, ysv (Yaroslav Savchuk) wrote…

Also I'm not sure how disabled state should work for relayers. Maybe it should affect only users but if transfer happened relayers should be able to provide evidences ? Shall we discuss this ?

Not sure I understand your first question. We have a test here where we disable an XRPL token and the relayer sends an evidence of a user sending the token and it fails, this covers your 2nd question. We also have another test where user sends a Coreum token which is disabled and also fails.

Regarding the logic it's in the update_token_state function added in this PR that disables it and the check is in each send with

if token.state.ne(&TokenState::Enabled) {
return Err(ContractError::XRPLTokenNotEnabled {});
}

Every time we send a token it checks if its enabled or not.

Regarding your second comment, relayers run the re-scan every day and if token is re-enabled they will just re-send the evidences and token will be successfully sent (if it was re-enabled). Basically the TX won't go through until we re-enable the token and once we do it will eventually go through, which is the expected behavior I believe. We can discuss or clarify if you want.

---

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

Previously, ysv (Yaroslav Savchuk) wrote…

nit: when I see update_token_state I expect method to also set new value in store. But since it just sets attribute value here maybe better name is smth like set_token_state ?

OK with that. Changed.

---

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

Previously, ysv (Yaroslav Savchuk) wrote…

nit: better error name: TokenStateIsImmutable "current token state is immutable"

Changed.

---

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

Previously, ysv (Yaroslav Savchuk) wrote…

nit better error name: InvalidTargetTokenState

you can also rename local variable to target_state if u like

Renamed both.

[dzmitryhil]

Reviewed 1 of 5 files at r4.
Reviewable status: 8 of 12 files reviewed, 7 unresolved discussions (waiting on @miladz68, @wojtek-coreum, and @ysv)

[dzmitryhil]

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

[ysv]

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

---

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

Previously, keyleu (Keyne) wrote…

Not sure I understand your first question. We have a test here where we disable an XRPL token and the relayer sends an evidence of a user sending the token and it fails, this covers your 2nd question. We also have another test where user sends a Coreum token which is disabled and also fails.

Regarding the logic it's in the update_token_state function added in this PR that disables it and the check is in each send with

if token.state.ne(&TokenState::Enabled) {
return Err(ContractError::XRPLTokenNotEnabled {});
}

Every time we send a token it checks if its enabled or not.

Regarding your second comment, relayers run the re-scan every day and if token is re-enabled they will just re-send the evidences and token will be successfully sent (if it was re-enabled). Basically the TX won't go through until we re-enable the token and once we do it will eventually go through, which is the expected behavior I believe. We can discuss or clarify if you want.

if token.state.ne(&TokenState::Enabled) {  
return Err(ContractError::XRPLTokenNotEnabled {});  
}

now it is clear. I expected this logic to be added in current PR
But since it is already present then I'm ok

Will add another comment with a suggestion

---

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

        .unwrap();

        // If we send second evidence it should fail because token is disabled

nit:
As an idea to consider.
I used to work with ActiveRecord (Ruby on Rails) and thingy which I really like in it is scope.

Here is a simple example:

class Book < ApplicationRecord
  scope :in_print, -> { where(out_of_print: false) }
  scope :out_of_print, -> { where(out_of_print: true) }
end

So you can call Book.in_print or Book.out_of_print etc.

And I consider this very useful in our case for COREUM_TOKENS & XRPL_TOKENS.
It would be nice if we can call smth like COREUM_TOKENS.enabled() as method or func where user interacts with contract.

This way we are protected against forgetting to call:

if token.state.ne(&TokenState::Enabled) {  
return Err(ContractError::XRPLTokenNotEnabled {});  
}

[ysv]

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

---

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

Previously, ysv (Yaroslav Savchuk) wrote…

nit:
As an idea to consider.
I used to work with ActiveRecord (Ruby on Rails) and thingy which I really like in it is scope.

Here is a simple example:

class Book < ApplicationRecord
  scope :in_print, -> { where(out_of_print: false) }
  scope :out_of_print, -> { where(out_of_print: true) }
end

So you can call Book.in_print or Book.out_of_print etc.

And I consider this very useful in our case for COREUM_TOKENS & XRPL_TOKENS.
It would be nice if we can call smth like COREUM_TOKENS.enabled() as method or func where user interacts with contract.

This way we are protected against forgetting to call:

if token.state.ne(&TokenState::Enabled) {  
return Err(ContractError::XRPLTokenNotEnabled {});  
}

https://guides.rubyonrails.org/active_record_querying.html#scopes

Also for users disabled token is same as it doesn't exist IMO.

And for admin we can call use COREUM\_TOKENS directly

[keyleu]

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

---

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

Previously, ysv (Yaroslav Savchuk) wrote…

if token.state.ne(&TokenState::Enabled) {  
return Err(ContractError::XRPLTokenNotEnabled {});  
}

now it is clear. I expected this logic to be added in current PR
But since it is already present then I'm ok

Will add another comment with a suggestion

It was there already because we Enable them when trust set is Accepted. Else they remain "Inactive". Remember we had a discussion about having 4 states instead of 2 some time ago.

---

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

Previously, ysv (Yaroslav Savchuk) wrote…

https://guides.rubyonrails.org/active_record_querying.html#scopes

Also for users disabled token is same as it doesn't exist IMO.

And for admin we can call use COREUM\_TOKENS directly

OK, since I will make a PR soon with extra tests modifications will spend some time looking into this!