-
Notifications
You must be signed in to change notification settings - Fork 249
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
Cryptocurrency advanced protobuf [ECR-2296] #1077
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1077 +/- ##
==========================================
- Coverage 92.91% 92.89% -0.02%
==========================================
Files 161 161
Lines 24115 24144 +29
==========================================
+ Hits 22406 22429 +23
- Misses 1709 1715 +6
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The proper way to 'link' against *.proto files is still an open question – I think – but I'm fine with using this current approach with environment variables. It's an implementation detail and we can make it better later without breaking compatibility.
Nicely done 👍
@@ -16,14 +16,19 @@ circle-ci = { repository = "exonum/exonum" } | |||
|
|||
[dependencies] | |||
exonum = { version = "0.9.0", path = "../../../exonum" } | |||
exonum_derive = { path = "../../../exonum_derive" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a reminder that we need to (eventually) publish all the dependencies on crates.io.
} | ||
|
||
impl CreateWallet { | ||
#[doc(hidden)] | ||
pub fn sign(name: &str, pk: &PublicKey, sk: &SecretKey) -> Signed<RawTransaction> { | ||
Message::sign_transaction(CreateWallet::new(name), CRYPTOCURRENCY_SERVICE_ID, *pk, sk) | ||
Message::sign_transaction( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could fit it in one line somehow? These parameters don't seem to be that important and long to stand out this much.
let tx = Self { name: name.to_owned() };
Message::sign_transaction(tx, CRYPTOCURRENCY_SERVICE_ID, *pk, sk)
Ditto for other transactions.
let path = env::current_dir() | ||
.expect("Failed to get current dir.") | ||
.join("src/encoding/protobuf/proto"); | ||
println!("cargo:protos={}", path.to_str().unwrap()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, this definitely is an abuse of this Cargo field, but if there is an upstream issue in Cargo to track it then I'm fine with treating this as an acceptable workaround.
Though, there is an alternative proposed by @slowli offline: export Protobuf content via constants for Rust modules, keep them as files for non-Rust code. However, this requires cooperation from protoc generator, it should be able to consume Protobuf strings from memory. Alternatively, we can export paths to the files as constants (doing exactly the same what's being done here at the moment, but without abusing environment variables).
use std::env; | ||
|
||
fn main() { | ||
let exonum_protos = env::var("DEP_EXONUM_PROTOBUF_PROTOS").unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm... maybe we should consider making this a part of exonum_build::protobuf_generate
. It's highly likely that all Exonum services will need Exonum Protobuf definitions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one I agree, I was thinking to do that since we hide exact implementation of the way we pass information about .proto file names.
Overview
See: https://jira.bf.local/browse/ECR-2296
Definition of Done