search

near / NEPs

The Near Enhancement Proposals repository
https://nomicon.io
208 stars 137 forks source link

NEP-330 extension: Build details extension #533

Closed Canvinus closed 5 months ago

Canvinus commented 7 months ago

NEP-330: Source Metadata

1.2.0 - Build Details Extension

Overview

This update introduces build details to the contract metadata, containing necessary information about how the contract was built. This makes it possible for others to reproduce the same WASM of this contract. The idea first appeared in the cargo-near SourceScan integration thread.

Benefits

This NEP extension gives developers the capability to save all the required build details, making it possible to reproduce the same WASM code in the future. This ensures greater consistency in contracts and the ability to verify source code. With the assistance of tools like SourceScan and cargo-near, the development process on NEAR becomes significantly easier.

frol commented 7 months ago

Thank you @Canvinus for submitting this NEP.

As a moderator, I reviewed this NEP and it meets the proposed template guidelines. I am moving this NEP to the REVIEW stage and would like to ask the @near/wg-contract-standards working group members to assign 2 Technical Reviewers to complete a technical review (see expectations below).

Just for clarity, Technical Reviewers play a crucial role in scaling NEAR ecosystem as they provide their in-depth expertise in the niche topic while work group members can stay on guard of the NEAR ecosystem. The discussions may get too deep and it would be inefficient for each WG member to dive into every single comment, so NEAR Developer Governance designed this process that includes subject matter experts helping us to scale by writing a summary with the raised concerns and how they were addressed.

Technical Review Guidelines * First, review the proposal within one week. If you have any suggestions that could be fixed, leave them as comments to the author. It may take a couple of iterations to resolve any open comments. * Second, once all the suggestions are addressed, produce a Technical Summary, which helps the working group members make a weighted decision faster. Without the summary, the working group will have to read the whole discussion and potentially miss some details. Technical Summary guidelines: * A recommendation for the working group if the NEP is ready for voting (it could be approving or rejecting recommendation). Please note that this is the reviewer's personal recommendation. * A summary of benefits that surfaced in previous discussions. This should include a concise list of all the benefits that others raised, not just the ones that the reviewer personally agrees with. * A summary of concerns or blockers, along with their current status and resolution. Again, this should reflect the collective view of all commenters, not just the reviewer's perspective. Here is a [nice example](https://github.com/near/NEPs/pull/399#issuecomment-1462492128) and a template for your convenience: ``` ### Recommendation Add recommendation ### Benefits * Benefit * Benefit ### Concerns | # | Concern | Resolution | Status | | - | - | - | - | | 1 | Concern | Resolution | Status | | 2 | Concern | Resolution | Status | ``` Please tag the @near/nep-moderators once you are done, so we can move this NEP to the voting stage. Thanks again.
lachlanglen commented 7 months ago

This looks useful. Any thoughts on adding commit_hash while you're at it to remove ambiguity of version field?

version: a string that references the specific commit hash or version of the code currently deployed on-chain.

Having a field that might be one thing or something else is not super clear IMO. I personally have added commit_hash in my own implementation of this standard for my contracts, as I find it useful to have both a version (e.g. v1.0.0) and a commit hash.

I guess a downside here is that it wouldn't be backwards-compatible in instances where the version field has been populated with a commit hash.

Canvinus commented 7 months ago

I totally agree with that point, @lachlanglen. I've just tried to retain as much as possible from previous contributors and ensure compatibility with already deployed contracts that have the metadata.

fadeevab commented 7 months ago

@Canvinus Thank you for the NEP!

(+@frol +@lachlanglen)

Allow me to share my overall perception:

  1. The structure may be overly flexible: all fields can be omitted.
  2. version could be mandatory, e.g. version is mandatory for public crates or NPM packages.
  3. What if we flatten the structure? For instance, by doing that, we can see that the contract_path can be merged into the link field, and probably tag/digest could be simplified (using a commit hash instead?).
  4. One more: ☝ build_command cannot guarantee obtaining exactly the same image with the same hash and properties because of dependencies and the compiler which may change over time (at least in minor versioning). However, the version + CI/CD release link with the permanently stored release build guarantees that we can look into the past: proven by having a security audit of EVERY update of the $USN contract while winding down the $USN project. Because GitHub Workflow contains the commit, test results, and the release binary, the hashes were compared to what's on the blockchain. Nevertheless, I admit the build_command can be useful overall.

Finally, let me share my thoughts about flattening the structure as a matter of discussion:

type ContractSourceMetadata = {
  version: string, // it could be mandatory, e.g. version is mandatory for publishing Rust crates, NPM packages
  source: string|null, // it could be the full path, e.g. https://github.com/my/repo/contract/src, in this case the `contract_path` is redundant
  commit: Hash|Tag|null, // commit "bf48943..." or "v0.1.0" tag (@lachlanglen)
  build: string|null, // build command or link to CI/CD build/release
  standards: Standard[]|null,
}
frol commented 7 months ago

One more: ☝ build_command cannot guarantee obtaining exactly the same image

@fadeevab That is why build_details is a combination of 4: "snapshot" reference to the source code, work dir where the build has happened withing the code snapshot, build command, and the docker image reference (name [tag] + unique identifier [digest]). You may want to review this comment.

@Canvinus I am not completely sold on your naming, but I would love to hear other opinions since I am obviously biased. I tried to keep the naming generic enough to allow some other solutions but Docker for env, and other solutions besides GitHub or even git (ipfs, tarball on s3, etc).

version could be mandatory, e.g. version is mandatory for public crates or NPM packages.

That would be a breaking change for NEP-330, and I don't see a good argument for that. If someone does not want to expose the version, it is up to them. It is really designed to be flexible. It is not a requirement to have this metadata to deploy your contract on the chain, right?

fadeevab commented 7 months ago

@frol

One more: ☝ build_command cannot guarantee obtaining exactly the same image

@fadeevab ... and the docker image reference (name [tag] + unique identifier [digest]). You may want to review this comment.

I didn't realize that:

  1. The "image" means a "docker image", not a "contract binary" in a raw form.
  2. The tag is a hub.docker.com image tag, not a git tag.
  3. The digest is the docker image digest.

(And right, tag/digest is just a Docker Hub way to encode an image).

Suggestions:

☝I see that the NEP is biased toward defining build environments via public docker images. 🤔However, I still can imagine not using BuildDetails and having just a link to a CI/CD public release to fit this NEP to my needs. 🙂Overall, I faced some confusion after jumping here from my bubble.

version could be mandatory, e.g. version is mandatory for public crates or NPM packages. If someone does not want to expose the version, it is up to them. That would be a breaking change for NEP-330

Here I saw that the NEP is on the review and my thoughts were that the NEP is not already adopted. Need to update the README's table.

It is really designed to be flexible. It is not a requirement to have this metadata to deploy your contract on the chain, right?

There is a case when the whole ContractSourceMetadata is empty. I was also thinking about making a version mandatory to make at least 1 field mandatory. I would also require developers to define

The bottom line is that the NEP can be improved with more clarification and examples.

P.S.: On a side note, I once had in my experience that a compiler attached a timestamp into a binary, so the digest never was the same. I'm not sure it's true nowadays for cargo/rustc. Nevertheless, I'm biased toward the GitHub Workflow+Release model which has proven to be simple and effective.

Canvinus commented 7 months ago

@Canvinus I am not completely sold on your naming, but I would love to hear other opinions since I am obviously biased. I tried to keep the naming generic enough to allow some other solutions but Docker for env, and other solutions besides GitHub or even git (ipfs, tarball on s3, etc).

  /// Reference to a reproducible build environment, e.g. Docker image reference:
  /// "docker.io/sourcescan/cargo-near:0.6.0"
  build_environment_ref: Option<String>,

@frol, I think we should also check if a Docker image is the right one by using its digest. So, we could suggest using the image's "digest" instead of its "tag" by default. For example, [REPOSITORY]@[DIGEST] or ghcr.io/[REPOSITORY]@[DIGEST]. This method is simpler because we just need one string, and it could work for other approaches, not just Docker.

Also this won't be a problem by pulling via digest:

P.S.: On a side note, I once had in my experience that a compiler attached a timestamp into a binary, so the digest never was the same. I'm not sure it's true nowadays for cargo/rustc. Nevertheless, I'm biased toward the GitHub Workflow+Release model which has proven to be simple and effective.

Canvinus commented 7 months ago

@fadeevab, would having a "source_code_snapshot" string field as the source for the CI/CD release, along with a "build_environment_ref" as a string field that could point to the commit of that published release, meet your requirements?

fadeevab commented 7 months ago

@Canvinus I think I could use the link field and put the commit hash in the version field, so no additional fields is required. Also, commit can be found/recorded in the public release builds, e.g. https://github.com/DecentralBankDAO/usn/releases/tag/v2.3.4

robert-zaremba commented 7 months ago

EnvImage could be renamed to DockerImage.

Let's use Container. There can be other container images than hub.docker.com

robert-zaremba commented 7 months ago

I think I could use the link field and put the commit hash in the version field, so no additional fields is required.

@fadeevab I think it's good to keep both version and field. If a code is uploaded without a version control (which should be a very rare case), then version would just be human friendly semantic number (eg v1.0.1) and must be the same as package / crate version.

Canvinus commented 6 months ago

@frol, @fadeevab, @lachlanglen, @robert-zaremba, thank you everyone for your insightful reviews. It looks like I have applied all the suggestions that were presented here. The only thing left to do is to refine the naming so that it satisfies everyone and aligns with their vision.

frol commented 6 months ago

Recommendation

As a subject matter expert, I recommend @near/wg-contract-standards to approve this NEP extension. As the Contract Standards WG member, I lean towards approving this NEP extension.

Benefits

Concerns

# Concern Resolution Status
1 Are these details enough to have a reproducible build Consensus is that the combination of build environment snapshot, source code snapshot, build path, and build command is enough Resolved
2 Various naming considerations The agreement is reached: { build_info: { build_environment, source_code_snapshot, contract_path, build_command } } Resolved
fadeevab commented 6 months ago

Recommendation

@near/wg-contract-standards As the Contract Standards WG member, I lean towards approving this NEP extension after resolving the concerns in the table below.

Benefits

The standard extension effectively adds a build_info to the contract metadata in a backward-compatible manner.

type ContractSourceMetadata = {
    version: string|null,
    link: string|null,
    standards: Standard[]|null,
+    build_info: BuildInfo|null, // optional, details required for contract WASM reproducibility.
}

The NEP provides examples of how the fields of BuildInfo can be customized and how build reproducibility can be achieved.

Concerns

# Concern Resolution Status
1 Considerations about BuildInfo structure complexity Strusture was flattened and simplified Resolved
2 How to properly use the NEP More details and examples added Resolved
3 Build reproducibility via Docker images Details about how to use build_environment and build_command Resolved
4 Concerns about 3rd party dependencies which affect reproducibility Suggest committing Cargo.lock Resolved[1]
5 Using public CI/CD releases instead of Docker environment as an option Using build_environment link to a custom env Won't be done

[1]: Should be completed within the issue https://github.com/near/cargo-near-new-project-template/issues/9

robert-zaremba commented 6 months ago

Recommendation

@near/wg-contract-standards As the Contract Standards WG member, I lean towards approving this NEP extension after resolving the Cargo.lock suggestion form @fadeevab

Concerns

# Concern Resolution Status
1 be more specific about optional and required fields spec updated Resolved
2 be more specific about version and link field link is an URL to version of repository when available Resolved
3 consider changing field names discussed and renamed Resolved
victorchimakanu commented 6 months ago

NEP Status (Updated by NEP Moderators)

Status: Approved

SME reviews:

Contract Standards Work Group voting indications (❔ | 👍 | 👎 ):

fadeevab commented 6 months ago

@victorchimakanu we still have unfinished Cargo.lock issue https://github.com/near/NEPs/pull/533#discussion_r1523848009 @Canvinus what's the status?..

Canvinus commented 6 months ago

@fadeevab, I talked with @frol about it, and they will update cargo-near-new-project-template to include Cargo.lock. As soon as that happens, I will include the latest commit of cargo-near-new-project-template as an example.

fadeevab commented 6 months ago

@Canvinus @frol Agree! I created a tracking issue https://github.com/near/cargo-near-new-project-template/issues/9 (don't forget it!), let's move forward with this NEP

victorchimakanu commented 5 months ago

As the moderator, I would like to thank the author @Canvinus and @alexzinin for submitting this NEP, and the NEAR contracts standards work group members for their reviews. Based on the voting indications above, it seems like this proposal is close to a decision. We are therefore scheduling the Fifth Contract standards Work group meeting next week, where this NEP can enter the final voting stage. Anyone can discuss the technical details by adding your comments to this discussion. And join the call to learn about the final decision and how to get more involved.”

Meeting Info

NEP Discussion: NEP-330 📅 Date: Thursday April 18th 2024 5PM UTC ✏️ Add to Calendar

fadeevab commented 5 months ago

@Canvinus It makes sense to fix a linter error Wrong url: https://rust-random.github.io/rand/rand/distributions/weighted/alias_method/struct.WeightedIndex.html Good url: https://rust-random.github.io/rand/rand/distributions/struct.WeightedIndex.html

Canvinus commented 5 months ago

@Canvinus It makes sense to fix a linter error Wrong url: https://rust-random.github.io/rand/rand/distributions/weighted/alias_method/struct.WeightedIndex.html Good url: https://rust-random.github.io/rand/rand/distributions/struct.WeightedIndex.html

Looks like it still has bunch of linter errors)

fadeevab commented 5 months ago

@Canvinus It makes sense to fix a linter error Wrong url: https://rust-random.github.io/rand/rand/distributions/weighted/alias_method/struct.WeightedIndex.html Good url: https://rust-random.github.io/rand/rand/distributions/struct.WeightedIndex.html

Looks like it still has bunch of linter errors)

Oh, right :) Usually, a markdown formatter in VS Code fixes 99% of those issues for me, e.g. placing spaces around headers. I guess, you should try.

frol commented 5 months ago

Thank you to everyone who attended the Contract Standards Working Group meeting today! The working group members reviewed the NEP and reached the following consensus:

Status: Approved (Meeting Recording: https://youtu.be/JQu8tObzhlM)

@Canvinus Thank you for championing the extension to NEP-330

@fadeevab @robert-zaremba Thank you for all the review!

Next steps: