Fix status checker, comparator and fetcher #99
Conversation
- Fixed status pull for lib projects - Fixed status comparator for app projects
Implemented dependencies methods in StatusChecker, StatusComparator and StatusFetcher to verify local and on chain dependencies status
Refatored proxy functions in StatusChecker and StatusFetcher to make them clearer
Tested dependencies behavior in both StatusFetcher and StatusComparator models
jbcarpanelli
force-pushed
the
fix/status-command
branch
from
cfab0a3
to
86f8f85
Compare
jbcarpanelli
force-pushed
the
fix/status-command
branch
from
86f8f85
to
dd0f999
Compare
| @@ -63,34 +69,33 @@ export default class StatusChecker { | |||
| } | |||
|
|
|||
| async checkVersion() { | |||
| const observed = (await this.app()).version | |||
| const observed = this._project.version | |||
| const expected = this.networkFile.version | |||
There was a problem hiding this comment.
I don't think this test is working properly: the project's version is set in the constructor, which is set here in L36 with the value of networkFile.version, so it will never fail. The observed value should actually be taken by querying project.getDependencyVersion(networkFile.name).
There was a problem hiding this comment.
You are right, L72 won't work for lib projects, but as far as I understand, we are not validating the version (and we were not validating it in the old model 🤔) for lib projects. That being said, we should totally check it! But I would prefer to do it in a future PR as it would be a "new feature".
| this.data.proxies[fullname][index] = fn(this.data.proxies[fullname][index]); | ||
| } | ||
|
|
||
| proxyFromIndex(thepackage, alias, index) { |
There was a problem hiding this comment.
Though we are not enforcing it everywhere, we're often using leading underscore to denote private functions in classes. Could you add them for these new helper methods, so proxyFromIndex becomes _proxyFromIndex and so forth? This way it's clearer which functions are part of the public interface, and which aren't.
| const expected = this.networkFile.contract(alias).bodyBytecodeHash | ||
| const observed = bytecodeDigest(web3.eth.getCode(address)) | ||
| const observed = bytecodeDigest(await promisify(web3.eth.getCode.bind(web3.eth))(address)) |
| package: event.args.package | ||
| })); | ||
|
|
||
| return _.uniqWith(filteredEvents, _.isEqual) |
There was a problem hiding this comment.
This may return repeated packages. For instance, if an app registers under the name "OZ", a package at 0x20 with version "1.0", and later registers under the same name a package at 0x20 with version "2.0", this method returns two different events. Since the "truth" is only the latest event (ie the one with version "2.0"), this would always yield a mismatch between the current version in the network.json file (which should correctly read "2.0"), and the old event (with version "1.0").
Instead of using uniqWith isEqual, we should use a comparator that checks only name, and ensures that the element with the highest index in the array (ie more recent) is kept. Using a reduce that builds a map indexed by name, and then returns the values on the map, should do the trick.
| }) | ||
|
|
||
| testVersion(); |
There was a problem hiding this comment.
Do you have a linter that removes semicolons, or it's just OCD? :-P
facuspagnuolo
added
topic:eth-packages
jbcarpanelli
force-pushed
the
fix/status-command
branch
from
a630a76
to
6ddc0b0
Compare
|
@jcarpanelli there are some files conflicting, would you mind fixing those? |
StatusChecker,StatusComparatorandStatusFetcherclasses to work with the new contracts architecture and lib models implemented by @spalladino to support multiple dependencies.