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

Gateway for deploying smart contract and mutation event parsing #522

Merged
merged 73 commits into from Jul 30, 2019

Conversation

@MoMannn
Copy link
Member

MoMannn commented Jun 28, 2019

TODO before merge:

  • spell check (@typwrtr)
  • code review (@xpepermint)
  • smart contract audit (@fulldecent)
  • add asset ledger deploy option
  • add value ledger deploy option
  • update documentation
  • add event parsing
  • fix readme with updated package names
@MoMannn MoMannn requested review from xpepermint, fulldecent and typwrtr Jun 28, 2019
@MoMannn MoMannn self-assigned this Jun 28, 2019
@MoMannn MoMannn changed the title Deploy gateway Gateway for deploy smart contract and mutation event parsing Jul 24, 2019
@MoMannn MoMannn changed the title Gateway for deploy smart contract and mutation event parsing Gateway for deploying smart contract and mutation event parsing Jul 24, 2019
Copy link
Member

xpepermint left a comment

Just add this this._logs=[]

@MoMannn MoMannn added this to the v2 (Aragorn) milestone Jul 25, 2019
Copy link
Member

fulldecent left a comment

Review scope

I have completed a code-level review of this PR. This includes manually reading the code and comparing to advertised or assumed functionality and best practices. Any finding that was previously referenced and which is still relevant to this code is referenced again below, so this is “fresh review”.

Excludes:

  • **/npm-shrinkwrap.json

  • **/*.min.js

Findings

Most findings are referenced in code (i.e. GitHub commit comments) here but everything is summarized here.

  • 💣 Address 0x000... used when a deployed contact is expected

    • Documentation states that code will be updated to point to a deployed contract but actually address 0x000… is used.
  • 💣 ​No policy for merging to master

    • The project security support policy states that all code in master is fully supported and eligible for bug bounty. Instead of merging to master we should maintain a separate prerelease branch from master and target this PR to there. After merge then an npm release can be made with a prerelease (alpha, beta, RC) version name. When the same code is ready to be released then master can be fast-forwarded against the prerelease branch. This entire process should be documented as an official policy. Alternatively, the policy can be lessed to only support the latest NPM non-prerelease release. Compare this to another popular project where they do all kinds of development in master, and none of the code ever works and everybody copy-pastes from it and gets hacked.
  • 💣 Did not test for correctness of dist files, this should be done automatically, #523

  • References in documentation are made to a separate deployment gateway

    • This is legacy code that no longer applies. Also, these documentation can please be updated to link to the contract interface. Example

    BAD: abcde | the abcde variable points to an ERC-721 contract

    GOOD: abcde | the abcde variable points to an ERC-721 contract

  • Vue plugins should use TypeScript instead of JavaScript

    • Vue supports TypeScript natively and 0xcert Framework is a TypeScript project
    • packages/0xcert-vue-example/plugins/0xcert.js
  • Dcoumentation examples do not compile (related to deployment gateway)

    • Examples in-line in GitHub review

    • Recommend to create test scripts named documentation-XXX and copy paste from there to documentation to ensure they compile

  • TokenCustom name/documentation does not accurately relate to content

    • "An example contract implementation" does not accurately reflect that this is the default value token deployed in the token deployment gateway.
    • Also, the contract name is similarly disconnected from its use.
  • Constant literals should not be defined in code

    • Literal values should not be created in-line in the source code. Instead they should be extract as a constant value and referenced where needed. Some values are calculated, like the hash value of an empty array. Alternatively, the hash value should be switched from a literal to a calculated value.
    • The Ethereum zero address 0x0000...
    • Hash value e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855
    • 0x8edda873a8ad561ecebeb71ceb3ae6bcb70c2b76a3fcb869859895c4d4fc7416
    • 0xdd97b854c02f699ea0d8984479d0012fbbbd0f4f80fc2e099315f6c47a3da178
    • 0x421b43caf093b5e58d1ea89ca0d80151eda923342cf3cfddf5eb6b30d4947ba0
    • 0x492318801c2cec532d47019a0b69f83b8d5b499a022b7adb6100a766050644f2
  • Solidity files have disallowed whitespace

    • Solhint should be included in travis test suite

    • See NFTokenTransferProxy and more

  • Solidity contracts could use tighter typing (Xcert create proxy vs. nftoken transfer proxy vs createproxy)

    • iproxy should be renamed, and a proxy should be created for the other actions
    • The keyword address should be used very rarely in the smart contracts.
      • Latter should have an interface too
      • assetCreateProxy
      • nftoken-safe-transfer-proxy can use ERC721 instead of address and recipient should be an ERC721TokenReceiveer
  • Some of the referenced public contracts are outdated

Other notes

  • After merged and released, be sure to mark the old NPM packages as deprecated since some packages were renamed
    • @0xcert/wanchain-order-gateway => @0xcert/wanchain-gateway
    • @ethereum-order-gateway => @0xcert/ethereum-gateway
    • @0xcert/ethereum-order-gateway-contracts => @0xcert/ethereum-gateway-contracts
docs/api/ethereum.md Outdated Show resolved Hide resolved
docs/api/ethereum.md Outdated Show resolved Hide resolved
docs/api/ethereum.md Outdated Show resolved Hide resolved
docs/api/ethereum.md Outdated Show resolved Hide resolved
docs/api/ethereum.md Outdated Show resolved Hide resolved
requiredConfirmations: 1,
gatewayConfig: buildGatewayConfig(NetworkKind.ROPSTEN),
});
ctx.is(provider.gatewayConfig.multiOrderId, '0x0000000000000000000000000000000000000000');

This comment has been minimized.

Copy link
@fulldecent

fulldecent Jul 25, 2019

Member

Constant should be defined and referenced from elsewhere

require(!deployCancelled[claim], DEPLOY_CANCELED);
require(!deployPerformed[claim], DEPLOY_ALREADY_PERFORMED);

deployPerformed[claim] = true;

This comment has been minimized.

Copy link
@xpepermint

xpepermint Jul 29, 2019

Member

Plase explain.

docs/api/ethereum.md Outdated Show resolved Hide resolved
docs/api/ethereum.md Outdated Show resolved Hide resolved
docs/api/ethereum.md Outdated Show resolved Hide resolved
docs/api/ethereum.md Outdated Show resolved Hide resolved
docs/api/ethereum.md Outdated Show resolved Hide resolved
docs/api/ethereum.md Outdated Show resolved Hide resolved
docs/api/ethereum.md Outdated Show resolved Hide resolved
MoMannn added 3 commits Jul 30, 2019
@MoMannn MoMannn changed the base branch from master to 2.0 Jul 30, 2019
@xpepermint xpepermint merged commit d141618 into 0xcert:2.0 Jul 30, 2019
3 checks passed
3 checks passed
codecov/patch 85.79% of diff hit (target 79.32%)
Details
codecov/project 82.88% (+3.56%) compared to 503d05f
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@MoMannn MoMannn deleted the MoMannn:deploy-gateway branch Oct 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.