Feat: Eth wallet contract implementation#10850
Conversation
birchmd
added
C-enhancement
| arc-swap = "1.5" | ||
| assert_matches = "1.5.0" | ||
| async-trait = "0.1.58" | ||
| aurora-engine-transactions = { git = "https://github.com/aurora-is-near/aurora-engine.git", tag = "3.6.1" } |
There was a problem hiding this comment.
Is there a reason these are a git dependency?
We should avoid git dependencies where possible. The fact that this is referencing a tag suggests to me that these should be on crates.io.
There was a problem hiding this comment.
Aurora doesn't have a regular process for releasing crates to crates.io. So for now a git dep is required to use the crate. But this is easy to update in the future if Aurora does start releasing crates.
There was a problem hiding this comment.
I think it should really be a not "if Aurora starts", but rather "integrating with the rest of the ecosystem ought to be a very strong incentive for Aurora to start releasing crates."
We can't really audit git tags, they are not reproducible and might just make the build process slower every time as cargo can't cache git repositories globally.
Does it block this PR? Probably not. But waiting for Aurora to realize these problems on their own before an action is taken is also not great.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #10850 +/- ##
==========================================
+ Coverage 71.37% 71.39% +0.02%
==========================================
Files 760 760
Lines 152772 152772
Branches 152772 152772
==========================================
+ Hits 109039 109075 +36
+ Misses 39207 39172 -35
+ Partials 4526 4525 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
birchmd
force-pushed
the
eth-wallet-contract
branch
from
c37b4d6 to
abfc04b
Compare
|
I started reviewing this but it might take a while. I plan to send feedback tomorrow. |
nagisa
left a comment
nagisa
left a comment
There was a problem hiding this comment.
Q for Adam: do we need to have the contract checked-in? Why aren't we building it on demand and e.g. verifying its reproducibility through “other” means?
For wallet tests lets see if we can (re-)use the infrastructure in near-test-contracts. At least that way all the relevant stuff stays in the same spot. Ideally these contracts would get built on the fly like test-contract-rs is, but its not a too big of a deal if doing so is really difficult.
I don't feel qualified to review the contract itself, and I don't think I have the capacity at the moment to become able. It looks like Adam is already on the hook for that anyhow, but please definitely talk with N1 EMs to prioritize it accordingly if that's not the case.
|
|
||
| [[package]] | ||
| name = "borsh" | ||
| version = "0.10.3" |
There was a problem hiding this comment.
Please investigate upgrading this dependency.
There was a problem hiding this comment.
This has been done on the Aurora side. But it hasn't been released as a tag yet on GitHub. This will be resolved when we next upgrade the Aurora dependency.
|
|
||
| [[package]] | ||
| name = "sha2" | ||
| version = "0.9.9" |
There was a problem hiding this comment.
Hmph. I know we don't do a best job of keeping nearcore's dependencies up-to-date ourselves, but this was one merge button away…
|
@staffik any updates on the review? |
There was a problem hiding this comment.
Great work, looks impressive!
Apologies for delay, we are absorbed in statelessnet issues.
I did not check test code yet and will look into the issue of checking-in the contract.
I am not qualified to review the contract too, but I heard there will be a proper audit later.
There are 3 comments left and I will quickly look at test-related code now. |
Sorry, I underestimated, will finish tomorrow morning. |
5 participants
This PR adds the eth wallet contract implementation which will be automatically deployed with eth implicit accounts. This is as per the design outlined in near/NEPs#518 .
It is intentional that the wallet contract implementation is isolated as its own crate and not contained in the broader nearcore workspace. This allows it to be reviewed, tested and audited independently from the rest of the Near code. The crate includes integration tests written using
near-workspaces. However, the nearcore integration test related to the wallet contract has also been updated to check that the contract is automatically deployed when creating an eth address and that it works as expected.This will not be the last PR in this project because I am only adding the logic for the implementation. It still remains to setup the reproducible build pipeline and update the tests which check the contract hashes. Additionally, there are still a few details which need to be finalized: (1) the Ethereum chain ID that will be associated with Near and (2) the Near account ID that will have the eth address registrar contract deployed. The eth address registrar contract stores a reverse lookup of Ethereum-like address to Near account IDs. It is necessary for the wallet contract to detect faulty relayers.
However this (relatively large) PR can be reviewed and merged, and the points above will be addressed in much more manageable follow-up PRs.