feat: populate metadata with build information #55
Conversation
| @@ -51,6 +45,12 @@ pub(crate) fn run(args: BuildCommand) -> anyhow::Result<()> { | |||
| )?; | |||
| min_abi_path.replace(util::copy(&path, &out_dir)?); | |||
| } | |||
| contract_abi.metadata.build = Some(BuildInfo { | |||
There was a problem hiding this comment.
Shouldn't we do this before writing out the embedded version of the ABI?
There was a problem hiding this comment.
This is opinionated I guess. Do you think this information should be embedded inside of the wasm file?
There was a problem hiding this comment.
Unsure, but this, in a sense, is part of the contract's signature.
We could make it opt-in, though, so another flag for including build information in the embedded ABI?
There was a problem hiding this comment.
Ok, decided to include the build information in the end. It is only ~30 extra bytes anyway and we can always come up with a flag that disables it if there is a demand.
Also, probably wise to try and keep embedded and non-embedded ABI to look as close as possible.
| contract_abi.metadata.build = Some(BuildInfo { | ||
| compiler: format!("rustc {}", rustc_version::version()?), | ||
| builder: format!("cargo-near {}", env!("CARGO_PKG_VERSION")), | ||
| image: None, |
There was a problem hiding this comment.
Curious how we're going to inject this bit of information. Env var only set when built from contract-builder?
Because either way, that information needs to be known at this stage to be embedded in the wasm file.
There was a problem hiding this comment.
Env var only set when built from contract-builder?
Most likely. There no built-in way to get the docker image name from inside of the container from what I have researched.
There was a problem hiding this comment.
I wonder if sdk-js gets an equivalent of contract-builder at some point and what that would mean for this field. I guess in a sense, the builder information plus the image helps better describe what type of contract-builder this is. But it's a non-issue, we have a long way ahead before we're at that point.
There was a problem hiding this comment.
I was actually wondering if it makes sense to "demote" contract-builder to only serve as an operating system and then just install whatever is necessary based on the build info (e.g. rustc 1.61.0, cargo-near 0.2.0). This way it could potentially work for both sdk-rs and sdk-js.
Also, presumably sometime in the future rustc/cargo will reach the point where wasm builds can be bit-by-bit reproducible and hedonistic and we will not need an image anymore.
There was a problem hiding this comment.
This is a good point, not sure what the size overhead would be from including the JS SDK in the same docker image, but it would certainly help unify things. Also, imho, the current docker image might be a little too big, I'll try to see if we can cut things down a bit, drop unnecessary binaries, strip some maybe etc. Doesn't seem like the official rust image makes much of an effort to reduce size anyway.
|
@miraclx I was waiting for 4.1.0-pre.4 release (expected it to come a bit sooner) to not have to bother with git revs, but will update & merge this PR to unblock you. |
| @@ -12,7 +12,7 @@ serde = { version = "1", features = ["derive"] } | |||
| schemars = "0.8" | |||
|
|
|||
| [patch.crates-io] | |||
| near-sdk = { git = "https://github.com/near/near-sdk-rs.git", rev = "792d5eb26d26a0878dbf59e304afa4e19540c317" } | |||
| near-sdk = { git = "https://github.com/near/near-sdk-rs.git", rev = "792d5eb26d26a0878dbf59e304afa4e19540c317", git = "https://github.com/near/near-sdk-rs.git", rev = "6d73c9ff4fd095fc23eaa000c14ab65c15c4aa6b" } | |||
There was a problem hiding this comment.
Hahaha, interesting that this is still a valid toml syntax
There was a problem hiding this comment.
I am working on a PR right now that will allow us not to update all the toml files every time we upgrade the SDK version, so will fix there
There was a problem hiding this comment.
Nice! Can't wait to see it.
Fixes #19
Depends on near/near-sdk-rs#927