Move Polkadot implementation to Rust #3857
Conversation
| vec![network as u8] | ||
| }, | ||
| _ => { | ||
| let first = ((network & 0b0000_0000_1111_1100) as u8) >> 2; |
There was a problem hiding this comment.
These magic values can be declared as const globally
rust/tw_ss58_address/src/lib.rs
Outdated
|
|
||
| let bytes = match key { | ||
| PublicKey::Ed25519(k) => k.as_slice(), | ||
| _ => return Err(AddressError::InvalidInput), |
There was a problem hiding this comment.
AddressError::PublicKeyTypeMismatch suits here too.
Also, personally I prefer to take a strict public key type in the address implementation, so tw_keypair::ed25519::sha512::PublicKey, and do the tw::PublicKey -> tw_keypair::ed25519::sha512::PublicKey conversion in entry.rs implementation.
For example,
wallet-core/rust/chains/tw_greenfield/src/entry.rs
Lines 66 to 69 in de287c8
| address: &str, | ||
| _prefix: Option<Self::AddressPrefix>, | ||
| ) -> AddressResult<Self::Address> { | ||
| PolkadotAddress::from_str(address)?.with_network_check() |
There was a problem hiding this comment.
We should allow to verify Kusama, Acala and other addresses via the Polkadot implementation. To achieve this, it's worth to check the address prefix if it's provided and SS58
| } | ||
|
|
||
| lazy_static! { | ||
| static ref CALL_INDICES_BY_NETWORK: HashMap<NetworkId, CallIndicesTable> = call_indices! { |
There was a problem hiding this comment.
I think it's worth to move the "standard" call indices to a dedicated module like src/extrinsic/standard_extrinsic.rs as well as encode_transfer, encode_asset_transfer etc, so to make src/extrinsic/mod.rs more general and re-usable.
| // `Extrinsic` is (for now) just a lightweight wrapper over the actual protobuf object. | ||
| // In the future, we will refine the latter to let the caller specify arbitrary extrinsics. |
There was a problem hiding this comment.
I personally prefer to separate business logic (low-level blockchain implementation) and TrustWallet Protobuf interface. Please consider adding a src/extrinsic_builder.rs or src/extrinsic_protobuf.rs that will try to convert Protobuf messages to Extrinsics.
| let address = | ||
| SS58Address::from_str(&t.to_address).map_err(|_| EncodeError::InvalidAddress)?; |
There was a problem hiding this comment.
Recently, we added error contexts to add some useful info to the SigningOutput.error_message string: #3806
To be able to add an error context, please consider changing the EncodeResult type to Result<T, TWError<EncodeError>>.
| // TODO: check address network ? | ||
| let address = | ||
| SS58Address::from_str(&b.controller).map_err(|_| EncodeError::InvalidAddress)?; |
There was a problem hiding this comment.
That would be great but not required. In Cosmos we also ignored the address networks because of the complexity. It's also a responsibility for the API user to validate input addresses
| } | ||
| } | ||
|
|
||
| // TODO: implement U128 support (we don't have it yet in tw_number) |
There was a problem hiding this comment.
u128 and i128 are standard Rust types, but it would be useful to implement some functions that we have for U256, I256
| }; | ||
| } | ||
|
|
||
| tuple_impl!(T0, T1, T2, T3, T4, T5, T6, T7); |
There was a problem hiding this comment.
Do we really need to implement that trait for tuple of 7 items? :D
Tbh I like the macro, but I find it difficult to maintain. Do you think we can just implement the Scale coding for a simple tuple? Do we even need it?
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
Good test coverage 👍
| message_oneof: Proto::mod_SigningInput::OneOfmessage_oneof::polymesh_call( | ||
| Proto::PolymeshCall { | ||
| message_oneof: Proto::mod_PolymeshCall::OneOfmessage_oneof::identity_call( | ||
| Proto::Identity { | ||
| message_oneof: Proto::mod_Identity::OneOfmessage_oneof::add_authorization( | ||
| Proto::mod_Identity::AddAuthorization { | ||
| call_indices: Some(Proto::CallIndices { | ||
| variant: Proto::mod_CallIndices::OneOfvariant::custom( | ||
| Proto::CustomCallIndices { | ||
| module_index: 0x07, | ||
| method_index: 0x0d, |
There was a problem hiding this comment.
Highly nested code. Could you please move some parts to functions?
For example,
fn custom_call_indices(module: u8, method: u8) -> Option<Proto::CallIndices> {
Some(Proto::CallIndices {
variant: Proto::mod_CallIndices::OneOfvariant::custom(
Proto::CustomCallIndices {
module_index: module as u32,
method_index: method as u32,
// ...
}| pub fn leading_zeros(&self) -> u32 { | ||
| self.0.leading_zeros() |
There was a problem hiding this comment.
Just curious what the value is 🤔
Description
How to test
Types of changes
Checklist
If you're adding a new blockchain