Skip to content
This repository has been archived by the owner on Jan 24, 2022. It is now read-only.

Link solidity libraries #208

Merged
merged 13 commits into from
Oct 15, 2018
Merged

Link solidity libraries #208

merged 13 commits into from
Oct 15, 2018

Conversation

jbcarpanelli
Copy link
Contributor

@jbcarpanelli jbcarpanelli commented Oct 9, 2018

Fixes #85

This is a first approach for linking solidity libs before deploying logic contracts.
Implemented so far:

  • Solidity libs discovery
  • Solidity libs linking
  • Solidity libs tracking (in zos.network.json)
  • Fixed zos status ... --fetch command to compare implementations as both logic contracts and solidity libs

Still have to:

  • Add tests
  • Fix skipped tests
  • Finish fixing the zos status ... --fix command
  • Recursive solidity libs discovery

@spalladino spalladino self-assigned this Oct 9, 2018
@jbcarpanelli jbcarpanelli added the status:in-progress Under development, do not merge this PR label Oct 9, 2018
@spalladino spalladino force-pushed the feature/link-solidity-libraries branch 3 times, most recently from cc908e2 to b5714ff Compare October 11, 2018 22:22
@spalladino spalladino force-pushed the feature/link-solidity-libraries branch from b5714ff to 762805a Compare October 12, 2018 11:28
@spalladino
Copy link
Contributor

@jcarpanelli please review the new commits!

@spalladino spalladino added status:to-review Awaiting review and removed status:in-progress Under development, do not merge this PR labels Oct 12, 2018
Copy link
Contributor

@facuspagnuolo facuspagnuolo left a comment

Choose a reason for hiding this comment

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

LGTM, no blocking comments :)

"networks": {},
"schemaVersion": "2.0.1",
"updatedAt": "2018-10-12T10:51:50.530Z"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it necessary to publish these build files?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actual stdlibs libs evm packags have their build artifacts published, so these ones for testing should have them as well.

@@ -1,6 +1,6 @@
import Truffle from '../models/truffle/Truffle';
import Session from '../models/network/Session'
import Contracts from 'zos-lib/lib/utils/Contracts';
import { Contracts } from 'zos-lib';
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

this._checkSolidityLibImplementationAddress(alias, address)
this._checkSolidityLibImplementationBytecode(alias, address, bytecode)
}
//TODO: implement missing remote solidity libs validation
Copy link
Contributor

Choose a reason for hiding this comment

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

can we move this to an issue?

Copy link
Contributor

Choose a reason for hiding this comment

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


// Verifies if a bytecode represents a solidity library.
export function isSolidityLib(bytecode) {
return bytecode.match(/^0x73[A-Fa-f0-9]{40}3014/)
Copy link
Contributor

Choose a reason for hiding this comment

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

we should add a comment explaining why we assume that expected bytecode to be a lib

await Promise.all(_.map(contracts, async ([contractClass, contractAlias]) => {
contractClass.link(libraries);
await project.setImplementation(contractClass, contractAlias);
}));
Copy link
Contributor

Choose a reason for hiding this comment

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

This code is not that clear, +1 to refactor it in a future

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, it's on my technical debt workflowy section

@@ -97,6 +97,7 @@ export default {
contract.setProvider(web3.currentProvider)
contract.defaults({ from: web3.eth.accounts[0], ... defaults })
contract.synchronization_timeout = syncTimeout
contract.setNetwork('test')
Copy link
Contributor

Choose a reason for hiding this comment

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

🙌

// run('rm contracts/*.sol ||:')
// run('rm zos.* ||:')
// run('rm package.* ||:')
// })
Copy link
Contributor

Choose a reason for hiding this comment

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

should we remove this after statement?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I unfortunately forgot to uncomment it. It's no harm though.

@spalladino spalladino merged commit cb31325 into master Oct 15, 2018
@facuspagnuolo facuspagnuolo deleted the feature/link-solidity-libraries branch October 15, 2018 16:38
@spalladino spalladino removed the status:to-review Awaiting review label Oct 15, 2018
spalladino added a commit that referenced this pull request Oct 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Libraries are not linked when running zos push
3 participants