-
Notifications
You must be signed in to change notification settings - Fork 111
feat(tron): initial tron HD activation support #2467
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
base: dev
Are you sure you want to change the base?
Conversation
onur-ozkan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I realized while reviewing this PR is that we are adding a lot of TODOs to the codebase. It's probably much better to create a tracking issue for them instead.
mariocynicys
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
first iteration
onur-ozkan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you.
| }, | ||
| EthCoinType::Nft { .. } => return MmError::err(WithdrawError::NftProtocolNotSupported), | ||
| EthCoinType::Nft { .. } => { | ||
| return MmError::err(WithdrawError::ProtocolNotSupported(format!( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this is not directly related to this PR,
but we have the ProtocolNotSupported error variant and at the same time a text description attached, which means the same. We could refactor such variants like that:
#[display(fmt = "Protocol {} not supported", _0)] // Such an attribute could be added for RPC-level errors
ProtocolNotSupported { protocol: String }
| }, | ||
| EthCoinType::Nft { .. } => { | ||
| MmError::err(BalanceError::Internal("Nft Protocol is not supported yet!".to_string())) | ||
| MmError::err(BalanceError::Internal("Nft protocol is not supported yet!".to_string())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here instead of BalanceError::Internal we could use a dedicated variant BalanceError::ProtocolNotSupported, like in other errors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its better to revise error variants in other pr
|
I have few questions:
|
This pr is not merged yet #2450
You are asking for TronClient support. It is in the plans. The node in the request is just a string with the komodo proxy flag (the array of nodes to be clear). For the Tron client, this request object should also be used to construct the transport layer. Since the current PR does not aim to provide full TronClient support, we are only allowing the code to build the Web3 instance for now. |
|
I will update tron checklist #1542 (comment) |
| if self.is_tron() { | ||
| // TODO use Tron client | ||
| let sun = U256::zero(); | ||
| warn!( | ||
| "Using stub implementation for Tron address_balance for {}, returning {sun} SUN", | ||
| format!("{:?}", TronAddress::from(&address)), | ||
| ); | ||
| return Ok(sun); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't we wanna do such a check in most places since we now merged CoinType::Eth into CoinType::TRX?
This pull request introduces initial support for the TRON (TRX) coin activation, targeting HD mode.
coin configuration for coins file:
To test fucntionaity
Set
"enable_hd": truein the KDF configuration.Use the
enable_eth_with_tokensrequest with "TRX" as the ticker.However, one thing in the HD activation response concerns me. I decided to reuse EthCoin for TRON support (since the keypair is fully compatible), but the HDAddress type for EthCoin is defined as
pub type EthHDAddress = HDAddress<Address, Public>;, which uses the Ethereum address format. As a result, the address shown in the activation response appears in Ethereum format.https://github.com/KomodoPlatform/komodo-defi-framework/blob/63043f3915f80977c0b287dfeaa0b66749902885/mm2src/coins/coin_balance.rs#L581-L588
Note: In the logs, I can see that the correct TRON address is generated from the seed (checked address with other crypto wallet which supports bip-32/39 and Tron), so the key derivation itself works as expected.
Address Compatibility Note
In the pr I dont enforce strict TRON-specific address format for swap addresses.
This is because TRON and Ethereum addresses are fully compatible at the byte level:
0xprefix.0x41, and then encoded in Base58Check for display.Essentially:
H1600x41 + H160(Base58Check encoded for display)This means the underlying data (H160) is the same, and TRON addresses can be derived from the same key pair as Ethereum.
However, I agree that we should enforce TRON-specific address format later in requests, to align with user expectations and network standards.