Skip to content
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

Feat(eth-wallet-contract): reproducible build #11011

Merged
merged 4 commits into from
Apr 12, 2024

Conversation

birchmd
Copy link
Contributor

@birchmd birchmd commented Apr 9, 2024

Continuing with the work on eth-wallet contract, this PR adds a reproducible build process. It uses the Docker image recommended in near-sdk-rs. Note that the reproducible build process is skipped if the wasm file is already present and it is still committed to the repo. This is intentional because it means developers not wanting to worry about the eth-wallets contract can safely ignore it entirely when setting up their development environment. However, it does mean that any changes to the wallet contract implementation will also require locally deleting the wasm file and then running the build process to generate the new wasm file. The test that checks the hash of the wasm file will also need to be updated with whatever the new hash is.

For reviewers: please try deleting the wasm file yourself and running the build process (e.g. with cargo test -p near-wallet-contract --features nightly) to ensure that it is indeed reproducible across machines.

This PR also sets the value of ADDRESS_REGISTRAR_ACCOUNT_ID equal to the agreed upon value address-map.near. This account exists on both testnet and mainnet so the same value can be used for both networks. For now Aurora holds the keys to this account, but once we are sure the address registrar contract implementation (present in the previous eth wallet PR) will not change (e.g. after it has been audited) then we will delete the keys from that account to lock the contract implementation.

@birchmd birchmd requested a review from staffik April 9, 2024 19:48
@birchmd birchmd requested a review from a team as a code owner April 9, 2024 19:48
@birchmd birchmd requested a review from akhi3030 April 9, 2024 19:48
Copy link

codecov bot commented Apr 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.24%. Comparing base (8c7f2a6) to head (a98d365).

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #11011      +/-   ##
==========================================
+ Coverage   71.22%   71.24%   +0.01%     
==========================================
  Files         760      760              
  Lines      152630   152630              
  Branches   152630   152630              
==========================================
+ Hits       108711   108738      +27     
+ Misses      39481    39449      -32     
- Partials     4438     4443       +5     
Flag Coverage Δ
backward-compatibility 0.24% <ø> (ø)
db-migration 0.24% <ø> (ø)
genesis-check 1.43% <ø> (ø)
integration-tests 36.83% <ø> (-0.07%) ⬇️
linux 69.73% <ø> (+0.01%) ⬆️
linux-nightly 70.70% <ø> (+<0.01%) ⬆️
macos 52.72% <ø> (-1.66%) ⬇️
pytests 1.66% <ø> (ø)
sanity-checks 1.45% <ø> (ø)
unittests 66.90% <ø> (+0.02%) ⬆️
upgradability 0.29% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@akhi3030 akhi3030 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving to unblock. I haven't actually reviewed. Best to wait for @staffik to do a pass.

Copy link
Contributor

@staffik staffik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
Can we add instruction cargo test -p near-wallet-contract --features nightly to README?
I am asking because cargo build --features nightly did not work because of include_bytes in read_contract.

runtime/near-wallet-contract/build.rs Outdated Show resolved Hide resolved
@birchmd
Copy link
Contributor Author

birchmd commented Apr 10, 2024

cargo build --features nightly did not work

@staffik could you elaborate more on the error you got? This command does work on my machine. It works whether or not runtime/near-wallet-contract/res/wallet_contract.wasm is present (when it is not present it is re-built from the logic in runtime/near-wallet-contract/build.rs)

@staffik
Copy link
Contributor

staffik commented Apr 10, 2024

cargo build --features nightly did not work

@staffik could you elaborate more on the error you got? This command does work on my machine. It works whether or not runtime/near-wallet-contract/res/wallet_contract.wasm is present (when it is not present it is re-built from the logic in runtime/near-wallet-contract/build.rs)

I've removed runtime/near-wallet-contract/res/wallet_contract.wasm and run cargo build --features nightly on MacOS.
This is my output:

% cargo build --features nightly
   Compiling near-store v0.0.0 (/Users/stafik/near/nearcore/core/store)
   Compiling near-client-primitives v0.0.0 (/Users/stafik/near/nearcore/chain/client-primitives)
   Compiling near-wallet-contract v0.0.0 (/Users/stafik/near/nearcore/runtime/near-wallet-contract)
   Compiling neard v0.0.0 (/Users/stafik/near/nearcore/neard)
   Compiling congestion-model v0.0.0 (/Users/stafik/near/nearcore/tools/congestion-model)
error: couldn't read runtime/near-wallet-contract/src/../res/wallet_contract.wasm: No such file or directory (os error 2)
  --> runtime/near-wallet-contract/src/lib.rs:14:16
   |
14 |     let code = include_bytes!("../res/wallet_contract.wasm");
   |                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: this error originates in the macro `include_bytes` (in Nightly builds, run with -Z macro-backtrace for more info)

error: could not compile `near-wallet-contract` (lib) due to 1 previous error
warning: build failed, waiting for other jobs to finish...

@birchmd
Copy link
Contributor Author

birchmd commented Apr 10, 2024

Hmm, that is strange. I do not know why the build logic would not trigger to create the missing file. I know it's time-consuming to do this, but when it's not an interruption for you to do so (like when you're about to go for a break or something), could you try running cargo clean and then cargo build --features nightly? Maybe the problem is somehow on MacOS it is not properly detecting that the missing file should trigger the build process to happen.

@staffik
Copy link
Contributor

staffik commented Apr 12, 2024

runtime/near-wallet-contract/res/wallet_contract.wasm

The same issue even when I did cargo clean.
I had similar issue on CI, see #10269 (comment).
And I used

    #[cfg(not(feature = "nightly"))]
    let code = &[];

@birchmd
Copy link
Contributor Author

birchmd commented Apr 12, 2024

Weird that even cargo clean does not help. I kind of expect the current solution not to work in CI because it uses a docker image and I think setting up the CI to run the docker image would take additional work.

I'm doing a bit of refactoring of the build process in the next PR I have planned. So I think I'll merge this PR for now and we can continue debugging this in the next PR.

@birchmd birchmd enabled auto-merge April 12, 2024 13:27
@birchmd birchmd added this pull request to the merge queue Apr 12, 2024
Merged via the queue into near:master with commit bd80b1d Apr 12, 2024
29 checks passed
@birchmd birchmd deleted the eth-wallet-contract-repo-build branch April 12, 2024 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants