transactions! macro #457

matklad · 2018-02-01T19:07:07Z

This PR changes the way services are defined.

This is the groundwork to implement #424 properly.

The main idea is that instead of message! macro, authors of servers use transactions! macro, which

declares a bunch of transactions in one go,
assigns message_id automatically,
creates a special transaction set type for working with any of the service's transactions.

alekseysidorov · 2018-02-02T11:47:10Z

exonum/src/blockchain/service.rs

+/// transactions! {
+///     const SERVICE_ID = SERVICE_ID;
+///
+///     MyTransactions {


Can you move const SERVICE_ID to the MyTransaction block? In my opinion it will be more consistent.

alekseysidorov · 2018-02-02T15:09:05Z

exonum/benches/block.rs

-            struct Tx {
-                const TYPE = 1;
-                const ID = 0;
+        transactions! {


And what about more "rusty" syntax? I think that this is quite possible to do.

transactions! { enum Transactions { const SERVICE_ID: u16 = 0; TxFirst { from: &PublicKey, data: &[u8] }, TxSecond { from: &PublicKey, time: SystemTime, } } }

It was one option I was considering, but I decided against it in the end:

The really important fact is that each of the transactions is a type on its own, it is really a standalone struct.

The fact that there's a wrapping enum is not really important: you never work with it as with an enum, you only ever use it in the boiler plate implementation of Service::tx_from_raw and when parsing transaction from JSON, but both of this places should be handled automatically once we parametrize Service over TransactionSet (as in #424).

On the other hand, it's quite nice that you impl Transaction on a case by case basis.

slowli · 2018-02-02T17:03:32Z

Assuming message_id field (either as a constant, or a method) would be removed from the transaction interface, would it still make sense to have a transactions! macro? IMO, in that case it would make sense to use something like

// within a separate `transactions` module
message! {
    struct CreateWallet {
        from: &PublicKey,
        name: &str,
    }
}

message! {
    struct Transfer {
        from: &PublicKey,
        to: &PublicKey,
        amount: u64,
        seed: u64,
    }
}

impl Transaction for CreateWallet { /* .. */ }
impl Transaction for Transfer { /* .. */ }

// note the absence of service id; same as with `message_id`,
// it's not the job of the transaction union to define it.
transaction_union! {
    enum Any {
        CreateWallet, // or, say, `CreateWallet(_)` for increased Rust-ness
        Transfer,
    }
}

This assumes that Any looks like a single transaction to the external world, but inside it discriminates among CreateWallet and Transfer variants, say, using the first 2 bytes of binary serialization or the message_type: "create-wallet" | "transfer" field in JSON (the spec is highly hypothetical here). transaction_union! takes care of assigning these tags to transactions; and it implements the "public" transaction interface. In this case, Any would be a single transaction for the service, so it could be an associated type or something like that if it simplifies things.

I wonder if something like this is realistically possible, though. For one, it would require rethinking transaction design completely; Transfer, Any and Any + service_id would be three different types, likely implementing different interfaces. So to create a "complete" Transfer transaction, it would be necessary to write something like

let tx = Any::Transfer(Transfer::new(..)).with_service_id(..);

matklad · 2018-02-05T11:10:08Z

@slowli I'd say that yes, transaction macro will stay in some form. In your example transaction_union is in a sense the transaction macro from this PR.

Ideally, each of individual transactions is just a #[dervie(Serialize, Deserialize)] struct, and wrapping it into an enveloped is done by the framework itself, so that the service implementor does not have to write let tx = Any::Transfer(Transfer::new(..)).with_service_id(..);.

One possible problem here is that signing a transaction would necessary need to violate layers :( When implementing a service, we don't care about message and service ids at all, but we must sign payload together with these ids.

So, I think a reasonable plan is to merge this PR in some form and than make improvements in two directions:

Try to move individual transactions out of the macro
Try to get rid of tx_from_raw and a POST endpoint for transactions

vitvakatu · 2018-02-05T12:08:37Z

I strongly ask for testing this PR with one of the private examples to make sure everything works as desired. Contact me if you need some help with it.

matklad · 2018-02-05T13:35:04Z

Indeed, the Debug impl was missing! Thanks, @vitvakatu! The stats for the PR with upgrade are +60 −172.

slowli

Couple of comments as of b6e9fdc. As per some previous discussions: Is there a particular reason not to make this change backward-compatible and move all new functionality into a separate module (e.g., blockchain::ext)? I feel that would be appropriate given that transactions! macro is expected to be quite volatile in the nearest future.

slowli · 2018-02-06T12:19:33Z

exonum/src/blockchain/service.rs

+/// `TransactionSet` trait describes a type which is an `enum` of several transactions.
+/// The implementation of this trait is generated automatically by the `transactions!`
+/// macro.
+pub trait TransactionSet


A bit nitpick-y, but: why is the trait called *Set? I'd say that it describes a tagged union (with variants tagged by message_id), or enum in Rust terms.

I am not too thrilled by Set myself, but I don't know of a better name :(

TransactionSet can represent either of a set of transactions, though, this is a type-level, and not value-level set.

slowli · 2018-02-06T12:28:15Z

exonum/src/blockchain/service.rs

+/// The implementation of this trait is generated automatically by the `transactions!`
+/// macro.
+pub trait TransactionSet
+    : Into<Box<Transaction>> + DeserializeOwned + Serialize + Clone {


Is Into<Box<Transaction>> really needed? Seems like the code compiles fine without it.

It is used here https://github.com/exonum/exonum/pull/457/files#diff-29918e4d0be4fa7170f31c0e3e9152f8R822 and is tested by doctest for Service.

If the supertrait is removed (but not impl Into<Box<Transaction>> for types generated by transactions!; I'm not disputing that it's needed), all lib tests and the Service doc test still work fine (at least, for me locally). So, the question is: is the supertrait (not implementation!) needed, and what does it convey logically? More generally, I've got an impression (maybe, a mistaken one - would be glad to be corrected!) that it's considered a best practice in Rust not to use supertraits unless completely necessary.

Hm, indeed 🤔

The idea is that one can write a generic tx_from_raw<S: TransactionSet> and a generic POST /transactions methods using only : TransactionSet bound, and not TransactionSet + Into. This is not done in this PR to reduce its scope, but the approach should be similar to #424.

Given that we generate TransactionSets via macros, and we always generate the Into impl, I don't think that the bound is particularly problematic.

However I'll try to take a holistic look at the trait hierarchy for Transaction, Message and StorageValue and see if something could be drastically simplified there!

slowli · 2018-02-06T12:33:37Z

exonum/src/blockchain/service.rs

+        }
+
+         impl<'de> $crate::encoding::serialize::reexport::Deserialize<'de> for $transaction_set {
+            #[allow(unused_mut)]


Seems like this attribute is obsolete; there is no muts in the method code.

slowli · 2018-02-06T12:40:28Z

exonum/src/messages/spec.rs

@@ -12,89 +12,75 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.

-/// `message!` specifies a datatype for digitally signed messages that can be sent


Do I understand correctly that it would be relatively easy to leave message! as is? If so, I'd recommend to do that in the light of the instability of transactions! macro that we've discussed previously.

slowli · 2018-02-06T12:47:47Z

exonum/src/messages/raw.rs

@@ -257,6 +262,19 @@ impl MessageWriter {
    }
 }

+// This is a trait that is required for technical reasons:
+// you can't make associated constants object-safe. This


Interestingly enough, if I understand correctly, the problem is with Transaction rather than Message. Arguably, Transaction (at least in the form implemented by services) should not inherit from Message at all; the first represents the wire format, the second - business logic.

matklad · 2018-02-06T13:55:09Z

Meta comment:

I think that we should either not break any user code at all, or we should proactively improve APIs, without leaving fallbacks. Any middle ground strategy seems to be strictly worse than either of the extremes :) I think that at this stage we are at the "improving API" stage.

With this in mind, I think that we should either decide that this is a better approach and use it instead the one with message! and manual implementation of tx_from_raw, or decide that the existing approach is better and stick with it, until we invent something better :)

It seems to me that transactions! approach would actually be more resilient to changes, because it hides service and message ids.

slowli · 2018-02-06T14:10:33Z

I think that we should either not break any user code at all, or we should proactively improve APIs, without leaving fallbacks.

I agree, but in this case this PR does not go far enough to fall into the "proactively improving API" category, imo. It leaves Transaction trait as is, when it arguably needs to be substantially refactored (e.g., separate business logic and wire format, i.e., Transaction and Message; remove service_id and message_id from Transaction; make services correspond one-to-one to TrasactionSet, etc.). Rather, it looks like the PR introduces some non-breaking improvements, and then breaks message! for the sake of it. This is the reason why I'm a bit hesitant, not because it breaks too much stuff.

matklad · 2018-02-06T14:20:14Z

I'd say it does improve the API measurably, because it reduces the amount of code user has to write (https://github.com/exonum/cryptocurrency-advanced/pull/99/files) and makes the typical bug "forget about a variant in tx_from_raw" harder to make.

Totally agree that transactions/services need improvement as a whole, but I see TransactionSet as an incremental step towards that direction, which nevertheless is valuable by itself. With transaction set, the next steps could be adding type Transactions: TransactionSet to the service, so as to make one-to-one correspondence and removing message_id (as now it is not explicitly exposed to the user). Hopefully the serde serialization by @vldm turns out great, which should allow us to be even more flexible with refactorings: currently, it's rather hard to modify anything message-related :(

slowli · 2018-02-06T14:45:49Z

OK, I got your point and mostly agree with it. Although I'd still leave message! intact if a) this wouldn't reduce the functionality of this PR; b) the message! macro will get obsoleted soon (due to changes in transaction serialization). Under these conditions, it seems kind of an ungrateful move to slightly tinker with macro just to prevent its use.

One more thing to understand your plans better: after a change to transaction serialization, it seems like the form of the transactions! could change as well to something like I've written previously, with variant types defined outside of the macro. Do you have something like this in mind, or are there objections to it?

matklad · 2018-02-08T13:04:38Z

Status: on hold, because the direction for services is unclear.

`TransactionSet` trait and `transactions!` macro allow to define a family of transactions for a service together, avoiding boilerplate like assigning message id or deserialization.

matklad requested review from vldm and stanislav-tkach February 1, 2018 19:07

matklad mentioned this pull request Feb 2, 2018

Experimental: automatically generate /transactions endpoint #424

Closed

alekseysidorov reviewed Feb 2, 2018

View reviewed changes

matklad force-pushed the protocol-v3 branch 3 times, most recently from a11308f to 7c60549 Compare February 2, 2018 14:21

alekseysidorov reviewed Feb 2, 2018

View reviewed changes

matklad force-pushed the protocol-v3 branch 2 times, most recently from d0c1e37 to 478b1a9 Compare February 2, 2018 15:49

matklad changed the title ~~WIP: transactions! macro~~ transactions! macro Feb 2, 2018

matklad requested a review from therustmonk February 2, 2018 16:14

stanislav-tkach requested review from aleksuss, slowli, ozkriff and vitvakatu February 6, 2018 09:20

slowli reviewed Feb 6, 2018

View reviewed changes

matklad and others added 4 commits February 9, 2018 20:56

Use a single macro to generate messages en-mass

d5f41fe

Minor: reflow doc comments

6909eea

Introduce TransactionSet

c51f18c

`TransactionSet` trait and `transactions!` macro allow to define a family of transactions for a service together, avoiding boilerplate like assigning message id or deserialization.

Update docs

e609655

Aleksey Kladov added 5 commits February 9, 2018 21:02

Update other crates to the transactions! macro

016b3da

Move service_id inside of the transactions block

1f8657a

Reformat code

dd14bff

Missing Debug impl

8248100

Drop unused allow(unused_mut)

8a3b5f5

matklad force-pushed the protocol-v3 branch from 2676704 to 56bf9f7 Compare February 9, 2018 18:20

Move TransactionSet to the transactions module

2eb8fd3

matklad force-pushed the protocol-v3 branch from 56bf9f7 to 2eb8fd3 Compare February 9, 2018 19:26

Merge branch 'master' into protocol-v3

13cdf36

stanislav-tkach approved these changes Feb 9, 2018

View reviewed changes

stanislav-tkach merged commit 9be11d8 into exonum:master Feb 9, 2018

stanislav-tkach mentioned this pull request Feb 9, 2018

Update changelog (transactions macro) #476

Merged

slowli mentioned this pull request Feb 15, 2018

Prepare for develop branch exonum/exonum-doc#178

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

transactions! macro #457

transactions! macro #457

matklad commented Feb 1, 2018

alekseysidorov Feb 2, 2018

alekseysidorov Feb 2, 2018

matklad Feb 2, 2018

slowli commented Feb 2, 2018

matklad commented Feb 5, 2018

vitvakatu commented Feb 5, 2018

matklad commented Feb 5, 2018

slowli left a comment

slowli Feb 6, 2018

matklad Feb 6, 2018

slowli Feb 6, 2018

matklad Feb 6, 2018

slowli Feb 6, 2018

matklad Feb 6, 2018

slowli Feb 6, 2018

slowli Feb 6, 2018

slowli Feb 6, 2018

matklad commented Feb 6, 2018

slowli commented Feb 6, 2018

matklad commented Feb 6, 2018

slowli commented Feb 6, 2018

matklad commented Feb 8, 2018

transactions! macro #457

transactions! macro #457

Conversation

matklad commented Feb 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slowli commented Feb 2, 2018

matklad commented Feb 5, 2018

vitvakatu commented Feb 5, 2018

matklad commented Feb 5, 2018

slowli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matklad commented Feb 6, 2018

slowli commented Feb 6, 2018

matklad commented Feb 6, 2018

slowli commented Feb 6, 2018

matklad commented Feb 8, 2018