Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(tokens): custom token activation for evm and tendermint #2141

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from

Conversation

shamardy
Copy link
Collaborator

No description provided.

@shamardy shamardy added the in progress Changes will be made from the author label Jun 12, 2024
Copy link
Collaborator

@mariocynicys mariocynicys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First review iteration

Comment on lines 65 to 67
CoinConfWithProtocolError::ProtocolMissMatch { ticker, .. } => {
InitL2Error::Internal(format!("Protocol from request is not supported for {}", ticker))
},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: We might want to print out the two protocols here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means that setting the protocol from a request is not supported for L2 as there is no custom tokens for L2, and we set it as none below. So no need to print out the protocol IMHO.

let (coin_conf_json, protocol_conf): (Json, L2::ProtocolInfo) = coin_conf_with_protocol(&ctx, &ticker, None)?;

from_config,
from_request
)]
ProtocolMissMatch {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: Mismatch, only a single s.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed here 7e95c7f

pub struct TendermintTokenProtocolInfo {
pub platform: String,
// Todo: can decimals be gotten from rpc call for custom tokens?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is denoms_metadata endpoint available on tendermint, but I am not sure if it works with the custom tokens.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the hint, will give it a look.

@@ -90,11 +91,13 @@ where
InitTokenError: From<Token::ActivationError>,
(Token::ActivationError, InitTokenError): NotEqual,
{
// Todo: we should check if the contract has been enabled before in case the user is re-enabling the coin using a different ticker
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds like an EVM specific logic, why checking it here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a misplaced todo, will move it to a better place.

Comment on lines 115 to 128
Err(err) => match protocol_from_request {
Some(protocol_from_request) => {
// Todo: Remove this once order matching using contract instead of ticker is implemented
conf["wallet_only"] = json::Value::Bool(true);
protocol_from_request
},
None => {
return MmError::err(CoinConfWithProtocolError::CoinProtocolParseError {
ticker: coin.into(),
err,
})
},
},
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

less confusing IMO

Suggested change
Err(err) => match protocol_from_request {
Some(protocol_from_request) => {
// Todo: Remove this once order matching using contract instead of ticker is implemented
conf["wallet_only"] = json::Value::Bool(true);
protocol_from_request
},
None => {
return MmError::err(CoinConfWithProtocolError::CoinProtocolParseError {
ticker: coin.into(),
err,
})
},
},
};
Err(err) => {
let protocol_from_request =
protocol_from_request.ok_or(CoinConfWithProtocolError::CoinProtocolParseError {
ticker: coin.into(),
err,
})?;
conf["wallet_only"] = json::Value::Bool(true);
protocol_from_request
},

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed here 8c489cf

@@ -104,6 +124,7 @@ impl From<BalanceError> for EnableTokenError {
#[derive(Debug, Deserialize)]
pub struct EnableTokenRequest<T> {
ticker: String,
protocol: Option<CoinProtocol>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we can call this custom_protocol?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The protocol is not custom though. The token is what is custom.

}
protocol_from_config
},
Err(err) => match protocol_from_request {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we work at all if "protocol" is missing in the 'coins' file?
I can see all coins there have "protocol" defined.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we can't enable a coin from config without a defined protocol.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in progress Changes will be made from the author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants