radicle-dev / radicle-registry Goto Github PK

Replace the term “call” with “message”. This includes function names, struct names, variables names, modules, etc. We also move the message structs in their own module. I.e. CreateCheckpointParams becomes message::CreateCheckpoint. (Already done previously)

In substrate upstream they recently updated the whole repository structure and renamed the crates.

Currently, we have two client (node and in-memory) and there are four packages that contain client logic: client, memory-client, client-interface, and client-common. Both clients share a substantial amount of code. This code is currently spread between client-interface, client-common or is simply duplicated in the client implementations. To address this issue I suggest the following.

We have one Client with multiple backends. The common client code would be part of the Client whereas the code that differs among the node and in-memory client would be part of a backend. The Client would be generic over a backend trait object. We would also collect all client related code in one package.

Calls to Error::unwrap are prevalent in the run implementation of the CLI commands. We should instead return these errors from run. This would require changing the error type returned by run.

async/await and core::future are stable in Rust. we should use them along with the futures-preview for the client.

License all code under GPLv3 and add that to the package Cargo.tomls

We want to provide a working ledger runtime that runs in memory and can be used programatically. Ideally we would have a client implementation that runs the substrate runtime in memory. We can than update and inspect the ledger through the client methods. This can be leveraged for testing, too.

We should investigate whether there are already ways to do this

Add some basic runtime tests (using the memory client) for transferring money and looking up balances.

We want to provide a downloadable version of the cli tools from bintray.

1. Create a new package on bintray
2. Adjust the ci upload script so that it also uploads the cli binary

client/src/call.rs uses sp_runtime::DispatchError, which is distinct from the DispatchError that the client exposes. This creates the scenario in which submitting a transaction may throw an error of an unaccounted-for type, forcing downstream consumers to import sp_runtime in order to account for the possibilities of transaction submission.

The Error type could solve this by encapsulating this case with a Dispatch variant which wraps sp_runtime::DispatchError, though this may be confusingly similar to the DispatchError that the client already exposes.

After #45, a project's ID is now a name/domain tuple, where both are Strings.

However, their length is not validated - it should not me more than 32 characters.

This should happen after the user provides them and before they are added to a block via the register_project transaction, in order to prevent problems with names that are too large.

Currently the Error type exported by radicle_registry_client is a type alias for substrate_subxt::Error. To reduce the dependency on the subxt library and to be able to properly document and extend the errors we should introduce our own error enum. This enum should implement From<substrate_subxt::Error>.

We add a metadata field of type Vec<8> to the project. The field is stored in the project state. We limit the size of the metadata to 128bytes.

The radicle_registry_client package has a large dependency footprint that makes it slow to compile. This has been raised as an issue in the substrate repo. I suspect that with the recent re-org of the repo this problem has been mitigated so #92 or #94 should fix this. We need to verify this and investigate further if the problem persists.

The polkadot app does not work with our node. This app is helpful for exploring and debugging the chain. We should investigate why it does not work and what we need to fix it.

At the moment we’re uploading node binaries for the radicle-registry-node bintray package for master and branch builds. This makes it hard to figure out what the latest master build is. To address this we should create another radicle-registry-node-dev package that only receives the branch build files.

Assume we register a project with a checkpoint that is not a root checkpoint, that is the checkpoint has a parent. A subsequent set_checkpoint transaction for the project with a descendant checkpoint of the initial checkpoint will fail.

It is more common in the Rust community to use dashes instead of underscores in package names. We should adhere to this.

The Client and ClientT methods should return v0.3 futures instead of v0.1 futures.

To improve CI build times we should use the LLD linker. Specifically, we should build the project with

RUSTFLAGS="-Clinker=/usr/bin/clang -Clink-arg=-fuse-ld=lld"

At the moment this does not seem to work with the WASM build runner. We need to investigate.

• set-checkpiont transaction
• set-contract transaction
• Transactions around domains (@geigerzaehler)
• Transactions around identity (@geigerzaehler)

We are not using the description and img_url fields for a project. We should remove them everywhere.

/cc @xla

Update rust-toolchain to a recent nightly. Also requires update of the CI base image and bumping the CI cache.

In order to decouple the runtime and the client's mutual dependency, the primitive datatypes present in the runtime should be moved to a separate package.

Needed by #121, related to #95.

Package aliases increase indirection in source code as they make it hard to see which actual dependency is being used.

With the recent Substrate package renames most crate names have become shorter, so package name aliases are no longer necessary.

The client library should check the runtime version of the chain when it connects to a node. If the runtime version is newer than the runtime version the client was compiled with then the client should return an error. This would prevent users from forgetting to run an updated client.

The runtime/tests/main.rs file is getting too large, and it is becoming difficult to discern how many and what things are being tested there.

Ideally, tests concerning the same behavior (e.g. transaction or data type implementation) are moved to their own module for clarity.

When registering we should require that the project domain is rad. Bonus: Do this as part of transaction validation in addition to runtime.

End-to-end tests that require a running node are expensive and have shared state. We want to test the full ledger behavior just using the runtime package. The tests should be of the form

1. submit transaction
2. observe output

In order to give out unique identifiers that can be used to inspect and query the state of a given transaction it would be helpful to retrieve the hash from new_signed call.

new_signed constructs a Transaction which is returned, I just want a way to get the hash of that instance. e.g.: `Transaction.hash.

Scope

This issue is meant to only tackle the implementation of the Org type, not it's integration with the other types neither transactions involving it.

For each package we intend to publish (runtime, node, client) we should create a README in the corresponding folder.

In order to be able to better debug the client library we should add logging with the log package to the code. For now, the main target would be the RemoteNode backend.

The documentation for String32 is not correct. The limit is based on the bytes in UTF-8 encoding and not on the number of unicode characters. We need to update the documentation for String32 and everywhere else.

Currently the errors returned for possible from conversions of vecs to Bytes128 return a String. This makes it hard to keep appropriate context when these errors are propagated. A proper error like the client exposes for other cases would be helpful.

We want to ensure that submitting invalid transactions results in submit_transaction returning an Invalid error. We need to write tests for this.

After #203

What

Projects in the registry must belong to an Org. Change the existing RegisterProject transaction to include the Org the project is being registered to.

• ProjectDomain is replaced with org ID (#199)
• Project name unique within the org
• Project is added to list of org projects
• Limit number of projects an org can have

Spec

After #138, the client has been moved to futures-0.3.
However, in tokio-0.2, the rokio::runtime::Runtime type no longer has an .executor() method as tokio::runtime::TaskExecutor has been removed.

As such, some parts of the client will need to be reworked to reflect this API change.

PR #119 raised the fact that common testing utilities are not very well implemented because they are not usable, by default, outside of the runtime crate.

This issue is about moving them into a separate crate so that tests in e.g. client can use them without include! manipulation.

At the moment the create_checkpoint method generates the checkpoint ID locally and submits it to the blockchain. However, the checkpoint ID should not be determined by the user. It should be deterministically generated and unique. There are two options to achieve this

• Use the transaction hash of the create_checkpoint transaction as the checkpoint ID. For this will need to make the transaction hash available to the runtime.
• Use substrates random number generator to generate the checkpoint ID and emit it as an event.

Add a node wait-sync command to the cli that waits until a node is fully synced and then exits. We could use this as the readiness probe on Kubernetes and not loose blocks when an update is deployed.

The funds will be moved to the Org level and thus the transfer transactions need to be adjusted.

• Drop TransferFromProject
• Adjust Transfer to support Orgs as the receipt account
• Adjust Transfer to support Orgs are the sender

Spec

We should expand the project registration implementation to match the spec. In particular we should implement project approval by root accounts and withdrawing project registration.

When creating a checkpoint, the provided project state hash should be unique with regards to the intended parent checkpoints i.e. it should be different from all of the state hashes in all of the checkpoints preceding the one that is being created.

A lot of tests in runtime/tests/main.rs use RegisterProjectParams with a ProjectId to register project. We should add a function that generates these parameters

fn random_register_project_params(checkpointId: Checkpoint) -> RegisterProjectParams

In order to make the client API simpler and easier to understand we want to introduce the following changes

• Rename ClientT::submit to ClientT::sign_and_submit_call
• Remove dedicated ClientT transaction methods that can be replaced by ClientT::sign_and_submit_call`. These methods are
  • ClientT::transfer
  • ClientT::register_project
  • ClientT::create_checkpoint
  • ClientT::set_checkpoint
    #131
• Make Client::create_with_executor() return a future instead of Result. Under the hood this method does IO and is blocking. #139

• Rename and move all implementations of Call. For example CreateCheckpointParams would be exposed as call::CreateCheckpoint. Creating transactions would look like this then

  Transaction::new_signed(author. call::CreateCheckpoint {...}, extra)
  

CI builds should fail if rust doc links (like /// [core::fmt::Display) cannot be resolved. This would increase the quality of the documentation and make it less likely that we forget to update documentation when things change.

I’m not sure how to achieve this but we need to deny the intra_doc_link_resolution_failure lint.