Port Polkadot implementation to Rust. #4102

Neopallium · 2024-11-08T18:03:42Z

Description

Continued the C++ -> Rust port of the Polkadot/Substrate blockchain from PR #3857.

Adds Rust crates:

tw_scale - SCALE encoding.
tw_ss58_address - SS58 address.
tw_substrate - Common code for supporting Substrate chains. Substrate chains can implement the SubstrateCoinEntry trait (see the tw_polkadot crate) instead of CoinEntry.
tw_polkadot - Polkadot/Kusama blockchain.

How to test

Types of changes

Checklist

Create pull request as draft initially, unless its complete.
Add tests to cover changes as needed.
Update documentation as needed.
If there is a related Issue, mention it in the description.

If you're adding a new blockchain

I have read the guidelines for adding a new blockchain.

Milerius · 2024-11-12T06:21:55Z

Great stuff @satoshiotomakan will take a look next week when he is back from PTO

rust/chains/tw_polkadot/src/entry.rs

satoshiotomakan

Thanks for opening the PR! First review iteration

include/TrustWalletCore/TWSS58AddressType.h

rust/Cargo.toml

rust/tw_substrate/src/lib.rs

rust/tw_scale/src/lib.rs

rust/tw_substrate/src/extrinsic.rs

rust/chains/tw_polkadot/src/call_encoder/polymesh.rs

satoshiotomakan · 2024-11-19T18:55:20Z

rust/chains/tw_polkadot/src/call_encoder/mod.rs

+            NetworkId::POLKADOT => PolkadotCallEncoder::new(ctx),
+            NetworkId::KUSAMA => KusamaCallEncoder::new(ctx),
+            NetworkId::POLYMESH => PolymeshCallEncoder::new(ctx),


While porting chains to Rust, we try to improve overall experience and codebase. Otherwise there is no much need in the migration.
We should handle this on the Entry level. So there should be separate tw_polkadot and tw_kusama crates, where you can, for example, provide a &dyn TWPolkadotCallEncoder object to a signer, or something like that.
Or it will be even better to provide a dynamic object that returns call indices for the asked extrinsic.

Please also note that we started using Polkadot blockchain implementation for all Substrate chains. For example, you can sign a transaction for Polymesh even if it's not in registry.json by using custom CustomCallIndices. But for breaking compatibility, we should use default values if the custom indices are not set for Polkadot or Kusama.

I think there is no need to add tw_polymesh chain, but rather add it to the registry.json file, and we'll use CustomCallIndices for every call.

I was planning on splitting tw_polkadot into per-chain crates later. For this PR I didn't want to make major structural changes to how the Rust vs C++ Polkadot backend worked.

For a lot of Substrate chains a single generic backend that allows CustomCallIndices would be enough, since they might only need Balance/Staking support. But for chains like Polymesh we have other calls that are needed to make using Trust Wallet work for our users.

Should I split tw_polkadot into 3 different chain impls (polkadot, kusama, polymesh) in this PR? Also we could have one generic Substrate chain crate that requires custom call indices to support chains that just need balance/staking support.

I have started work on moving Polymesh support into its own coin (tw_polymesh crate). Here is a draft PR for that: PolymeshAssociation#2

Right now it still uses the Polkadot protobufs definition, but we might want to make a new protobuf for Polymesh.

satoshiotomakan · 2024-11-26T10:38:52Z

Hi @Neopallium, is the PR ready for the next review iteration?

Neopallium · 2024-11-26T11:26:19Z

Hi @Neopallium, is the PR ready for the next review iteration?

I have some more changes (use TWError to improve errors) that I am in the middle of. Also a few more changes that are needed (from the unresolved comments). I will ping here when ready for the next review.

Neopallium · 2024-11-27T13:24:54Z

@satoshiotomakan Ready for the next review.

satoshiotomakan · 2024-12-06T17:28:55Z

Hi @Neopallium, thank you for letting me know! I'll review it next Monday, sorry for the delay

satoshiotomakan

Hi @Neopallium, thanks for the previous changes, appreciate the effort!
Last review iteration

rust/Cargo.toml

rust/chains/tw_polkadot/Cargo.toml

rust/chains/tw_polkadot/src/call_encoder/generic.rs

satoshiotomakan · 2024-12-09T18:02:55Z

rust/chains/tw_polkadot/src/call_encoder/mod.rs

+        // Special case for batches.
+        match msg {
+            SigningVariant::balance_call(b) => {
+                if let Some(batch) = self.encode_balance_batch_call(b)? {
+                    return Ok(batch);
+                }
+            },
+            SigningVariant::staking_call(s) => {
+                if let Some(batch) = self.encode_staking_batch_call(s)? {
+                    return Ok(batch);
+                }
+            },
+            _ => (),
+        }
+        // non-batch calls.
+        self.encoder.encode_call(msg)


I find these batch and non-batch procession suboptimal and difficult to understand.
The problem is that some calls are handled in CallEncoder (eg BondAndNominate), and later converted into separated Bond and Nominate, and then forwarded to a Substrate specific encoder.

However, developers need to keep in mind that in PolkadotCallEncoder you should return an error if BondAndNominate is passed. It's not obvious. Also encode_staking_batch_call confuses by returning optional value.

Could you please refactor it by providing a single structure responsible for Proto -> RawOwned conversion?
You can make it the way so tw_polymesh and tw_kusama can just reuse some functions of the function. Please take a look at Greenfield chain as an example:
https://github.com/trustwallet/wallet-core/blob/master/rust/chains/tw_greenfield/src/modules/tx_builder.rs

Please note that GenericBalances and GenericStaking look good to me

I find these batch and non-batch procession suboptimal and difficult to understand.
That is because of how the Polkadot.proto has special cases for batching. Batching should not be handled inside each pallet. On Substrate networks batching of calls is handled by the Utility pallet and each network can have some differences in that pallet.

I have tried multiple times to handle those special case batching inside the pallet (Balance, Staking), but it make all other simple call cases more complex.

In the next PR for Polymesh I have use a more generic batching method that simplifies the call encoding a lot:
Call encoder:
https://github.com/PolymeshAssociation/wallet-core/blob/polymesh_rust_impl/rust/chains/tw_polymesh/src/call_encoder.rs
Polymesh.proto:
https://github.com/PolymeshAssociation/wallet-core/blob/polymesh_rust_impl/src/proto/Polymesh.proto

The same designed could be used for Polkadot, but changing the proto file would break frontend clients.

rust/chains/tw_polkadot/src/call_encoder/polymesh.rs

rust/tw_scale/src/macros.rs

rust/tw_tests/tests/chains/polkadot/polkadot_address.rs

rust/tw_tests/tests/chains/polkadot/polkadot_sign.rs

tests/chains/Polkadot/TWAnySignerTests.cpp

tests/chains/Polkadot/TransactionCompilerTests.cpp

…mplementations

tests/chains/Polkadot/TWAnySignerTests.cpp

satoshiotomakan · 2024-12-13T16:23:00Z

Hi @Neopallium, please let me know once the PR is ready for the final review

Neopallium · 2024-12-13T17:32:12Z

@satoshiotomakan The only items remaining is:

Porting the C++ signing tests over to Rust. I still plan on doing this (keeping the same expected outputs), most likely next week.
The batching support. I still think we should be doing batching at the top-level. It would allow batching any set of calls, not just a limited few. Also there are other extrinsic calls (Multi-sig, committee proposals) that need to wrap another call. So it is better to handle that in a generic way.

Neopallium requested review from Milerius, satoshiotomakan and ar-g as code owners November 8, 2024 18:03

Neopallium changed the title ~~Polkadot rust impl~~ Port Polkadot implementation to Rust. Nov 8, 2024

Neopallium mentioned this pull request Nov 8, 2024

Move Polkadot implementation to Rust #3857

Draft

5 tasks

JaimeToca reviewed Nov 13, 2024

View reviewed changes

rust/chains/tw_polkadot/src/entry.rs Outdated Show resolved Hide resolved

Neopallium force-pushed the polkadot_rust_impl branch from 58cf478 to c5f078a Compare November 14, 2024 10:21

satoshiotomakan requested changes Nov 19, 2024

View reviewed changes

Neopallium force-pushed the polkadot_rust_impl branch from 19437b6 to a6558a7 Compare November 27, 2024 13:13

satoshiotomakan requested changes Dec 9, 2024

View reviewed changes

doom added 15 commits December 13, 2024 21:01

Add the tw_ss58_address crate

13c2d5d

Apply suggestions from code review, refactor some utility functions

0c5b2dc

Remove unused dependency

59f1894

Add PolkadotAddress

70775c0

Add NetworkId constants and Hash derivation

2a6a0fa

Add SCALE encoding for integers (fixed and compact)

fbc9bff

Fix tiny things in scale.rs

c09f789

Add extrinsics

3d41165

Add PolkadotPrefix

a56fb70

Require a pre-built NetworkId when deriving SS58Address

54b4a7e

Format extrinsic.rs

13b67d1

Add address parsing and derivation in entry.rs

ea6d34f

Rename SS58Address::from_str to SS58Address::parse

dc8565f

Add SubstrateNetwork to AddressPrefix enum

956b5f8

Rework ToScale trait to allow encoding to an existing Vec, add more i…

2c22292

…mplementations

Neopallium added 23 commits December 13, 2024 22:21

Fix memory leaks in C++ unit tests.

ae672d2

Update Acala support to match C++.

b09e9ff

Move tw_substrate to frameworks.

47874ba

Improve error reporting.

456f2d5

Simplify Compact encoding.

009a7b7

Refactor some rarely used SCALE code.

f08b80f

Rename Encoded to RawOwned and move to tw_scale.

142f1b1

Use H256 in Memo and IdentityId.

c4c1160

Don't store KeyPair.

2679356

Remove unused error variant.

b2342e8

Improve errors.

8d86d8d

Improve errors with context.

537b2c9

Remove TWSS58AddressType

d8566cf

Remove deadcode

2db5082

Remove consts from tw_ss58_address crate.

ef638d5

Simplify impl_enum_scale macro

3bcac4c

Document SCALE macros.

3786896

Some code cleanup and deps reorder.

eb1587a

Cleanup parse_address

11e5bdc

Fix RewardDestination. Added missing ACCOUNT variant.

0f0592f

Remove assert.

8217664

Fix C++ tests and use original expected output.

18a606f

Fix TODO

4c80040

Neopallium force-pushed the polkadot_rust_impl branch from e5466fa to 4c80040 Compare December 13, 2024 14:25

Neopallium commented Dec 13, 2024

View reviewed changes

tests/chains/Polkadot/TWAnySignerTests.cpp Show resolved Hide resolved

Neopallium added 3 commits December 13, 2024 14:58

Revert RewardDestination::ACCOUNT change.

083c345

Add links for Polymesh encoded transactions.

46862b9

Cleanup error conversion code.

7ea51a0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port Polkadot implementation to Rust. #4102

Port Polkadot implementation to Rust. #4102

Neopallium commented Nov 8, 2024 •

edited

Loading

Milerius commented Nov 12, 2024

satoshiotomakan left a comment

satoshiotomakan Nov 19, 2024

Neopallium Nov 21, 2024

Neopallium Nov 27, 2024

Neopallium Nov 28, 2024

satoshiotomakan commented Nov 26, 2024

Neopallium commented Nov 26, 2024

Neopallium commented Nov 27, 2024

satoshiotomakan commented Dec 6, 2024

satoshiotomakan left a comment

satoshiotomakan Dec 9, 2024

satoshiotomakan Dec 9, 2024

Neopallium Dec 13, 2024

satoshiotomakan commented Dec 13, 2024

Neopallium commented Dec 13, 2024

Port Polkadot implementation to Rust. #4102

Are you sure you want to change the base?

Port Polkadot implementation to Rust. #4102

Conversation

Neopallium commented Nov 8, 2024 • edited Loading

Description

How to test

Types of changes

Checklist

Milerius commented Nov 12, 2024

satoshiotomakan left a comment

Choose a reason for hiding this comment

satoshiotomakan Nov 19, 2024

Choose a reason for hiding this comment

Neopallium Nov 21, 2024

Choose a reason for hiding this comment

Neopallium Nov 27, 2024

Choose a reason for hiding this comment

Neopallium Nov 28, 2024

Choose a reason for hiding this comment

satoshiotomakan commented Nov 26, 2024

Neopallium commented Nov 26, 2024

Neopallium commented Nov 27, 2024

satoshiotomakan commented Dec 6, 2024

satoshiotomakan left a comment

Choose a reason for hiding this comment

satoshiotomakan Dec 9, 2024

Choose a reason for hiding this comment

satoshiotomakan Dec 9, 2024

Choose a reason for hiding this comment

Neopallium Dec 13, 2024

Choose a reason for hiding this comment

satoshiotomakan commented Dec 13, 2024

Neopallium commented Dec 13, 2024

Neopallium commented Nov 8, 2024 •

edited

Loading