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

Initialize Solang Aqd #2

Merged
merged 3 commits into from
Nov 11, 2023
Merged

Conversation

tareknaser
Copy link
Contributor

Packages

Package Description
aqd-core The CLI tool core crate
aqd-polkadot Smart contract interactions for Polkadot
aqd-solana Smart contract interactions for Solana
aqd-utils Utility functions and common code
aqd-solana-contracts Rust crate for Solana smart contract interactions

Copy link

@seanyoung seanyoung left a comment

Choose a reason for hiding this comment

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

I've tested it on Solana, and it works very nicely.

  • aqd solana deploy does not have the option of saving the generated private key
  • aqd solana call does not have a view option (maybe this should be aqd solana view)
  • There are still clippy warnings in rust 1.73.0

However I feel that these issues are details which can be resolved later. I am happy for this to be merged as is, the code is very nice.

Copy link

@xermicus xermicus left a comment

Choose a reason for hiding this comment

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

Looks good!

I'd just like to see some problems around CI and repo setup fixed, also there's a problem with non-mutating messages in Polkadot. The problem is that on the contracts pallet (runtime) level this isn't enforced, so users have to trust the client to not submit an extrinsic for messages considered immutable.

@@ -0,0 +1 @@
{"source":{"hash":"0x0cac9f6c156ce7cbc0bbd7ba4f1369aff039eee6d7e55a60855b63962780133d","language":"ink! 5.0.0-alpha","compiler":"rustc 1.73.0","wasm":"0x0061736d01000000011f0660027f7f0060000060047f7f7f7f017f60037f7f7f006000017f60017f00027406057365616c310b6765745f73746f726167650002057365616c301176616c75655f7472616e736665727265640000057365616c3005696e7075740000057365616c320b7365745f73746f726167650002057365616c300b7365616c5f72657475726e000303656e76066d656d6f72790201021003090804010005000001010608017f01418080040b0711020463616c6c000b066465706c6f79000c0ae807084d02017f027e230041206b2200240020004200370308200042003703002000411036021c20002000411c6a10012000290308210120002903002102200041206a2400410541042001200284501b0b12004180800441003b010041004102100a000b3c01027f027f200145044041808004210141010c010b410121024180800441013a000041818004210141020b2103200120023a000020002003100a000b8c0101057f230041106b22012400200142808001370208200141808004360204200141046a220441001009024020012802082205200128020c2202490d00200128020421032001410036020c2001200520026b3602082001200220036a360204200420001009200128020c220020012802084b0d00200320022001280204200010031a200141106a24000f0b000b4a01037f024002402000280208220241046a22032002490d00200320002802044b0d00200320026b4104470d012000280200210420002003360208200220046a20013600000f0b000b000b0d0020004180800420011004000b920301077f230041106b2200240002400240100541ff01714105470d0020004180800136020441808004200041046a100220002802042201418180014f0d00024020014104490d00418380042d00002102418280042d00002104418180042d000021050240418080042d00002203412f4704402001417c714104462003411d47722005413247200441ff017141e1004772722002419f0147720d02418480042802002104410121060c010b200541860147200441ff017141db004772200241d90147720d010b200042808001370208200041808004360204200041046a22054100100920002802082203200028020c2201490d01200028020421022000200320016b220336020420022001200120026a22012005100020032000280204220049722000410449720d012001280000210120060d02230041106b220024002000418080043602044180800441003a00002000428080818010370208200041046a20011009200028020c2200418180014f0440000b41002000100a000b410141011007000b000b200120046a1008410041001007000bcc0101057f230041106b2200240002400240100541ff01714105470d0020004180800136020c418080042000410c6a1002200028020c2200418180014f0d00024020004104490d00418380042d00002101418280042d00002102418180042d00002103418080042d0000220441e1004704402000417c714104462004419b014772200341ae01472002419d01477272200141de0047720d014184800428020010081006000b200341ef0147200241fe0047720d002001413e460d020b410141011007000b000b410010081006000b","build_info":{"build_mode":"Release","cargo_contract_version":"4.0.0-alpha","rust_toolchain":"stable-aarch64-apple-darwin","wasm_opt_settings":{"keep_debug_symbols":false,"optimization_passes":"Z"}}},"contract":{"name":"incrementer","version":"0.1.0","authors":["[your_name] <[your_email]>"]},"image":null,"spec":{"constructors":[{"args":[{"label":"init_value","type":{"displayName":["i32"],"type":0}}],"default":false,"docs":[],"label":"new","payable":false,"returnType":{"displayName":["ink_primitives","ConstructorResult"],"type":2},"selector":"0x9bae9d5e"},{"args":[],"default":false,"docs":[],"label":"new_default","payable":false,"returnType":{"displayName":["ink_primitives","ConstructorResult"],"type":2},"selector":"0x61ef7e3e"}],"docs":[],"environment":{"accountId":{"displayName":["AccountId"],"type":6},"balance":{"displayName":["Balance"],"type":9},"blockNumber":{"displayName":["BlockNumber"],"type":12},"chainExtension":{"displayName":["ChainExtension"],"type":13},"hash":{"displayName":["Hash"],"type":10},"maxEventTopics":4,"staticBufferSize":16384,"timestamp":{"displayName":["Timestamp"],"type":11}},"events":[],"lang_error":{"displayName":["ink","LangError"],"type":4},"messages":[{"args":[{"label":"by","type":{"displayName":["i32"],"type":0}}],"default":false,"docs":[],"label":"inc","mutates":true,"payable":false,"returnType":{"displayName":["ink","MessageResult"],"type":2},"selector":"0x1d32619f"},{"args":[],"default":false,"docs":[],"label":"get","mutates":false,"payable":false,"returnType":{"displayName":["ink","MessageResult"],"type":5},"selector":"0x2f865bd9"}]},"storage":{"root":{"layout":{"struct":{"fields":[{"layout":{"leaf":{"key":"0x00000000","ty":0}},"name":"value"}],"name":"Incrementer"}},"root_key":"0x00000000","ty":1}},"types":[{"id":0,"type":{"def":{"primitive":"i32"}}},{"id":1,"type":{"def":{"composite":{"fields":[{"name":"value","type":0,"typeName":"<i32 as::ink::storage::traits::AutoStorableHint<::ink::storage\n::traits::ManualKey<3794263404u32, ()>,>>::Type"}]}},"path":["incrementer","incrementer","Incrementer"]}},{"id":2,"type":{"def":{"variant":{"variants":[{"fields":[{"type":3}],"index":0,"name":"Ok"},{"fields":[{"type":4}],"index":1,"name":"Err"}]}},"params":[{"name":"T","type":3},{"name":"E","type":4}],"path":["Result"]}},{"id":3,"type":{"def":{"tuple":[]}}},{"id":4,"type":{"def":{"variant":{"variants":[{"index":1,"name":"CouldNotReadInput"}]}},"path":["ink_primitives","LangError"]}},{"id":5,"type":{"def":{"variant":{"variants":[{"fields":[{"type":0}],"index":0,"name":"Ok"},{"fields":[{"type":4}],"index":1,"name":"Err"}]}},"params":[{"name":"T","type":0},{"name":"E","type":4}],"path":["Result"]}},{"id":6,"type":{"def":{"composite":{"fields":[{"type":7,"typeName":"[u8; 32]"}]}},"path":["ink_primitives","types","AccountId"]}},{"id":7,"type":{"def":{"array":{"len":32,"type":8}}}},{"id":8,"type":{"def":{"primitive":"u8"}}},{"id":9,"type":{"def":{"primitive":"u128"}}},{"id":10,"type":{"def":{"composite":{"fields":[{"type":7,"typeName":"[u8; 32]"}]}},"path":["ink_primitives","types","Hash"]}},{"id":11,"type":{"def":{"primitive":"u64"}}},{"id":12,"type":{"def":{"primitive":"u32"}}},{"id":13,"type":{"def":{"variant":{}},"path":["ink_env","types","NoChainExtension"]}}],"version":"4"}

Choose a reason for hiding this comment

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

Why ink! contracts and not Solang? Ideally, this repo would just use some examples from Solang and compile them; would provide a more complete test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to use solang-compiled contracts with different versions of substrate-contracts-node, but encountered incompatibilities. The errors included:

Error uploading the code: Error decoding into dynamic value: Error at : Field deposit_held does not exist in our encoded data
  • Deploying solang-compiled contracts with the latest version of substrate-contracts-node failed during instantiation because of differing metadata formats.
Failed to deserialize ink project metadata from contract metadata
  • Even when attempting to use an older version of substrate-contracts-node, like v26, with solang-compiled contracts:
Error uploading the code: Error decoding into dynamic value: Error at : Field deposit_held does not exist in our encoded data

Currently, to test solang-aqd, I'm using ink!-compiled contracts with the most recent version of substrate-contracts-node. I'm using this approach until solang-substrate-ci gets updated.
The goal is to use solang-compiled contracts once this update is implemented.
The current setup helps make sure solang-aqd works well with Polkadot, handling interactions with Polkadot and providing accurate responses to user requests.

Copy link

@xermicus xermicus Nov 11, 2023

Choose a reason for hiding this comment

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

Currently, to test solang-aqd, I'm using ink!-compiled contracts with the most recent version of substrate-contracts-node

The current setup helps make sure solang-aqd works well with Polkadot, handling interactions with Polkadot and providing accurate responses to user requests.

This is obviously flawed. After all, solang-aqd is supposed to be tooling for solang and not ink!. The current setup does not make sure that solang-aqd works well with Polkadot; it only makes sure that solang-aqd works with some random pre-compiled ink! contract. Anyone trying to use solang-aqd will just find out that it's broken (despite the CI being green), which is obviously flawed.

I mean testing against an ink! contract is fine if your goal is to be compatible with ink! contracts too. But it's not the main goal here.

I'm using this approach until solang-substrate-ci gets updated.

This doesn't have anything to do with the node (if it had, obviously the solution would be to just use a compatible one on the CI). The contracts built by solang are fine and they run fine on all recent versions of pallet contracts.

As you already said. The problem is that aqd doesn't understand the metadata format (the node does not know anything about the metadata, the contracts are just binary blobs, they have some requirements but not on the metadata level). The real problem is that solang-aqd uses the unreleased version 5 of the metadata format, containing breaking changes; Solang contracts are built with version 4 of ink_metadata and do not include the staticBufferSize field in the metadata. Which is fine because this comes from an unreleased version of ink_metadata anyways (Solang uses the latest released version which is 4.3.0 and doesn't have this field yet).

If you encounter this problem, it's the wrong approach to just pretend everything is fine. The right way to do this is to state that the tooling does not yet work and to work on fixing this for good. This means you try to upgrade Solang to ink-5.0.0-alpha to make it work.

How this CI should be set up is to download a version of solang that is supposed to be compatible with your tool, then compile the contract on the CI and then use that to test everything.

We can merge this PR as it is, however

  • This needs to be addressed afterwards
  • The README should clearly state that the tool does not yet work for polkadot

Comment on lines 8 to 10
# Run every Monday at 1:30 AM UTC
schedule:
- cron: '30 1 * * 1'
Copy link

Choose a reason for hiding this comment

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

I'm not sure if we need this; on PRs and main should be enough. @seanyoung wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed it for now

.github/workflows/test.yml Show resolved Hide resolved

steps:
# Install jq to parse JSON (used in shell scripts)
- name: Install jq
Copy link

Choose a reason for hiding this comment

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

This is done for both Polkadot and Solana, could this be done in build instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the test workflow to be 3 separate jobs:

  • First one checks for formatting, linting and builds for all features
  • Second one to test solana node interactions
    • build the project using cargo build --no-default-features --features "solana" --release
  • Third one to test polkadot node interactions

I tried it in my github repository. Here is how the CI should look like when it's merged

Choose a reason for hiding this comment

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

LGTM

.github/workflows/test.yml Show resolved Hide resolved
crates/aqd-core/Cargo.toml Outdated Show resolved Hide resolved

/// macro to print a title (cyan and bold)
#[macro_export]
macro_rules! print_title {
Copy link

Choose a reason for hiding this comment

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

I'm not sure all of these macros needed (I guess could be functions instead), but we can leave them as is for now.

/// If the `execute` flag is set to `false`, it performs a dry run of the call and displays
/// the results. If the `output_json` flag is set to `true`, the output is in JSON format.
/// Otherwise, it prompts for a transaction confirmation and then submits the transaction for execution.
pub async fn handle(&self) -> Result<()> {
Copy link

Choose a reason for hiding this comment

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

There are "read only" messages in contracts. They'll have the mutates flag set to false in the metadata. For those messages, the -x/--execute should lead to an error instead.

crates/aqd-polkadot/src/commands/mod.rs Show resolved Hide resolved
crates/aqd-polkadot/src/commands/mod.rs Outdated Show resolved Hide resolved
This commit includes these changes:

- The README has been updated to include a basic usage guide.
- CI configuration has been optimized to build specifically for Solana and Polkadot features in their respective jobs.
- Restructured the dependencies, making Tokio optional and part of the Polkadot feature.
- Simplified the match statement for URL parsing in the Polkadot feature.
- Addressed and resolved Clippy warnings for improved code quality and consistency.

Signed-off-by: Tarek <[email protected]>
@tareknaser
Copy link
Contributor Author

I'm aware of some areas for improvement and development in the project. I've got a list of these issues that I'll open on GitHub after the initial PR is merged to keep track of them. Here's what needs addressing:

  • Using contract-extrinsics Dependency from crates.io Instead of Git
  • aqd should use solang compiled contracts with solang-substrate-ci in the CI (solang-substrate-ci needs to updated first)
  • Remove crate patches in the Cargo.toml file once Solana resolves these issues.
  • aqd-polkadot needs to print a warning if the node version it's trying to communicate with isn't compatible with the CLI version.
  • Write a function to do solana program close in rust similar to solana program deploy
  • The function decode_instruction_return_data in crates/aqd-solana-contracts/src/printing_utils.rs currently retrieves the return data from the logs due to a known bug in the Solana SDK, which is expected to be fixed in #33639. Once this fix is officially released, the function should be updated to fetch the return data directly from the transaction itself.
  • The file crates/aqd-utils/src/borsh_encoding.rs has been copied from solang. While solang provides the discriminator function, it doesn't expose BorshToken. Ideally, to reduce its reliance on solang (a heavy dependency), aqd should aim to be a lightweight crate. Instead, solang can depend on aqd-utils.
  • When displaying information for all possible instructions in Solana, the volume of data might make it challenging to read directly from the CLI. A solution could involve implementing tables to present each instruction's information. One potential crate suitable for this purpose is radicle-term.
  • For SolanaTransaction struct, there should also be a method to dry run the transaction (using rpc_client.simulate_transaction function).
  • Use Logging
  • Doc comments should include simple examples
  • aqd solana call does not have a view option (maybe this should be aqd solana view)
  • aqd solana deploy does not have the option of saving the generated private key
  • There's a problem with non-mutating messages in Polkadot. The problem is that on the contracts pallet (runtime) level this isn't enforced, so users have to trust the client to not submit an extrinsic for messages considered immutable.

@xermicus
Copy link

xermicus commented Nov 11, 2023

@tareknaser great, thanks for compiling this exhaustive list. Can you please open each one as issue in this repo so we don't loose track?

aqd should use solang compiled contracts with solang-substrate-ci in the CI (solang-substrate-ci needs to updated first)

This means upgrading Solang to use (yet unreleased) v5 of ink_metadata. Which unfortunately won't work just like that because this PR broke our use case. You can try to fix that in ink! v5 or I can add it to my backlog.

When displaying information for all possible instructions in Solana, the volume of data might make it challenging to read directly from the CLI. A solution could involve implementing tables to present each instruction's information. One potential crate suitable for this purpose is radicle-term.

Maybe could introduce verbosity level :) Also, sounds like it's related to the tracing functionality you have in mind?

@xermicus
Copy link

@tareknaser thanks, a lot of good stuff in here!

@xermicus xermicus merged commit 65f8b6a into hyperledger-solang:main Nov 11, 2023
1 check passed
@tareknaser tareknaser deleted the initial branch November 11, 2023 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants