trufflesuite / truffle-migrate

On-chain migrations management
17 stars 17 forks source link

Fixed caching bug: old Migration artifacts would overwrite newly deployed Migrations #18

Closed nadavhollander closed 6 years ago

nadavhollander commented 6 years ago

Was facing bug in which truffle migrate would never save the newly deployed Migrations.sol contract's artifacts, meaning that the entire migration sequence would run from the beginning every time truffle migrate was called.

After some digging, it turns out resolverIntercept.js is the culprit. In index.js, the following line results in the caching of an old, undeployed Migration artifact:

      var Migrations = resolver.require("./Migrations.sol");

However, if your first migration retrieves the Migration contract as artifacts.require('Migrations') and not artifacts.require('Migrations.sol'), the resolver will cache a duplicate, newer copy of the Migrations artifacts at the key Migrations alongside the one already cached at Migrations.sol.

This is problematic because the artifactor uses the resolver's cache to list out which artifacts need to be written to disk -- hence, in alphabetical order, the new artifact associated with the Migrations key is written first, and then overwritten by the old artifact associated with the Migrations.sol key.

This PR fixes the above by trimming away trailing import paths and .sol extensions from all cache keys.

pkieltyka commented 6 years ago

its really too bad truffle is split into a thousand packages because it makes it very difficult to debug. however, after much effort I've determined this PR specifically to break importing contracts from epm and npm packages -- that is: http://truffleframework.com/docs/getting_started/packages-npm no longer works since this PR

I was experiencing a bug which I reported here https://github.com/trufflesuite/truffle-resolver/issues/10 thinking it was the resolver, but in fact its the line of code from this PR:

import_path = path.basename(import_path, ".sol");

the issue is the import_path is overridden and removes the package_name, which is needed by this subsequent line:

var resolved = this.resolver.require(import_path);

(https://github.com/dharmaprotocol/truffle-migrate/blob/a6b216e5bad5b7b705e9a0be5c11977e266f3f1a/resolverintercept.js#L20)

in the truffle-resolver of the NPM code, https://github.com/trufflesuite/truffle-resolver/blob/develop/npm.js#L17

the package_name is parsed from the import_path, which in this case is being clobbered

just FYI, I've received a lot of feedback from other fellow Solidity developers with their unpleasant experiences with truffle as well and the amount of friction and extra effort from bugs like this. The project would benefit greatly from better engineering rigor, starting with test cases.

pkieltyka commented 6 years ago

the fix:

replace the line here: https://github.com/trufflesuite/truffle-migrate/blob/develop/resolverintercept.js#L11

with ...

import_path = import_path.replace(/\.sol$/i, '');