Skip to content
This repository has been archived by the owner on Jul 9, 2021. It is now read-only.

Remove owner from migrations #1533

Merged
merged 4 commits into from
Jan 31, 2019
Merged

Conversation

dekz
Copy link
Member

@dekz dekz commented Jan 21, 2019

Description

We interchanged owner and txDefaults.from. Internally the was always the same account so we never experienced too many issues. When using the migrations from CLI it can become confusing when the first account isn't the from account in txDefaults, i.e deploy the proxies from Account A, attempt to change permissions with Account B.

I've also added the txDefaults.from account into the AssetProxyOwner, and deduped if it is already account[0].

fixes #1491
fixes #1490

migrations λ ./bin/0x-migrate.js --from 0xe36ea790bc9d7ab70c55260c66d52b1eca985f84
(node:54498) [DEP0005] DeprecationWarning: Buffer() is deprecated due to security and usability issues. Please use the Buffer.alloc(), Buffer.allocUnsafe(), or Buffer.from() methods instead.
transactionHash: 0xf29f3f2306ea6813103da335df969bc0e25d282de8c575faba364aac0e5431c4
ERC20Proxy successfully deployed at 0x2b51cebdcf6b66abf3189bb0ab89844de6ff80cc
transactionHash: 0x23569c9e2a1e94d14c744ce8461c3b49aca84a8b7a3c9d93a7171f92ef5525ae
ERC721Proxy successfully deployed at 0xbdd57e931b07ff7c7f999da90a81195137a29bc8
transactionHash: 0x76f7b7c28c34283a24df1165aa65f8dd5d40183f7f3af56194315ead6f7e6e9a
ZRXToken successfully deployed at 0x9095a921e425aa58bd1f76522534106b74025b9c
transactionHash: 0x99a9b8775bd48f662c45b8f13f04dccad7ae86a08cd6a0794a2f680c2e4c5d28
WETH9 successfully deployed at 0xa6ef72d4abbab589e12b9d4aa58350ef1aa05694
transactionHash: 0xcc84075ddada5b412ccd519fbcd7819098fe95c46f26159c7cbaf4a293e22313
Exchange successfully deployed at 0x7dc9d8b6f8630edeeb872d0a8397dd295a49ce88
transactionHash: 0x3f77316b1524b8a9658186ab6bc65ca1fe8d3849cfcd872ebff18b2936038fa5
DummyERC20Token successfully deployed at 0x31fc396cbedb15b47860b0c17d1bd7c3ea4fe31f
transactionHash: 0xa2d3929a9c25778d95398e6f1ef0cdc42a24439350c351683bab100097805fde
DummyERC20Token successfully deployed at 0x52551883a9c9cc243354143f6d1705b9a13c1b82
transactionHash: 0xf6099f5525cc0f933fdd28fe0873790556c4e8544c54bdf7c7d3d5aece52cc15
DummyERC20Token successfully deployed at 0xe2b965b5c955e15afe7f9dc4b7c37c7c77ba5d76
transactionHash: 0x6c917ba2366227924cd976a5d8787ca84230f11a1241c5d2051764e8afa38138
DummyERC20Token successfully deployed at 0x213f349039f9ac63df227a1e8f6386c09f23e217
transactionHash: 0x4689b63a5d72b78d3497d1041f5f0b570a8865b6bc73c116e4117b459e0e4b46
DummyERC20Token successfully deployed at 0xfe2b22e2c2bea7b0235f54c60d5d85e98001b7a3
transactionHash: 0x748c9e39dc1fe5ca2109010aec3fa5cac3ce9b857f3fc2370f4ee2bb60f74465
DummyERC721Token successfully deployed at 0xcb2508989ddad6169c85eea0cd3d166db75d0396
transactionHash: 0xe9421e73c95c5961c7ba30784be2ba6391b7631396e57984f31283f6185e9776
MultiAssetProxy successfully deployed at 0x09e514058e8816fc3a75842de4f60e4c14e6c748
transactionHash: 0xa113ba4b8ef9c141adcb0a0da180d77a120bacc900bd352d3f3dccf8e94606c0
Forwarder successfully deployed at 0x3d386bd5ac7f2aec6ebf7ed1f3fc27d1e3efcdb6
transactionHash: 0xb91cce35f6621f19d4127bba6e5b28900aa061fd611595e864ad1f4b44e5edb1
OrderValidator successfully deployed at 0xc35fcecf2961a26835771e6c3fe87a6ab1d856e8
transactionHash: 0x5283076dbb41ab251e9cd710ba504356b991a3effc6944219457aab4bf055fc1
DutchAuction successfully deployed at 0xc0d8bc4bd38e61d9b02f1aac5d755edf9ef7bbfc
transactionHash: 0xb2c035781c76e87d8239ea8b9d620d7abc344a7f7031b38314e14423300191ff
AssetProxyOwner successfully deployed at 0xdfad778e8b4c17335ae3fa562c053e9b92205563

Output from migrations:

migrations λ yarn migrate:v2
transactionHash: 0xd2e8823b1cb7a920abe1eb25a1627c75d0af246a845c4f32056f4e34842667ff
ERC20Proxy successfully deployed at 0x1dc4c1cefef38a777b15aa20260a54e584b16c48
transactionHash: 0xec02cdaf9c287e661eed352cc9488649ff8acdc26677dba1571470c77aae63f3
ERC721Proxy successfully deployed at 0x1d7022f5b17d2f8b695918fb48fa1089c9f85401
transactionHash: 0x0c09f2785687b1e4c05c56bcc63bd5a149180a0ec66d1fcff982224e2ee01113
ZRXToken successfully deployed at 0x871dd7c2b4b25e1aa18728e9d5f2af4c4e431f5c
transactionHash: 0x2c4dc00fa9adddb6627364bfa3f81fb73aafbb74cd43dafbd67dc4492ab1691f
WETH9 successfully deployed at 0x0b1ba0af832d7c05fd64161e0db78e85978e8082
transactionHash: 0x1535e82ecd309adb89de7741ab8ddc4a9b10e7c02d3046307f88f97083015d98
Exchange successfully deployed at 0x48bacb9266a570d521063ef5dd96e61686dbe788
transactionHash: 0x25e69f3ca1fb205911841a6c7da9a67a3ea232e0af8f900af9b3414eca7417aa
DummyERC20Token successfully deployed at 0x34d402f14d58e001d8efbe6585051bf9706aa064
transactionHash: 0xc485b49f16f3b238a74bc1e015f42ca2dd4507c19dfb1350c33e63f2907168e8
DummyERC20Token successfully deployed at 0x25b8fe1de9daf8ba351890744ff28cf7dfa8f5e3
transactionHash: 0x2123a6718d7acd4ac3ff547474f04f13630fad060531951f61af0474de6956e5
DummyERC20Token successfully deployed at 0xcdb594a32b1cc3479d8746279712c39d18a07fc0
transactionHash: 0x580ee1f8a9a54675c8fda4dc2979706cda71f96ecd99415387a9dc03ce125fa7
DummyERC20Token successfully deployed at 0x1e2f9e10d02a6b8f8f69fcbf515e75039d2ea30d
transactionHash: 0x7b949bb750f7aaace0a77cbf108e416342753d2004f8e4ed257abe25d08cb1fe
DummyERC20Token successfully deployed at 0xbe0037eaf2d64fe5529bca93c18c9702d3930376
transactionHash: 0x0746f3e4dab2d9076e59ef3d422acae3d011935cbd75d112602222498ee47df8
DummyERC721Token successfully deployed at 0x07f96aa816c1f244cbc6ef114bb2b023ba54a2eb
transactionHash: 0xf25b57439510c8cd7b7e6af9706d6464dbd009fee9e2391847e76bf68ee375b5
MultiAssetProxy successfully deployed at 0x6a4a62e5a7ed13c361b176a5f62c2ee620ac0df8
transactionHash: 0x3b1a1d30f59eb1ecdfe1ab56dc8cf4284bc91d438c0b526115fcb1cf3781ff30
Forwarder successfully deployed at 0x6000eca38b8b5bba64986182fe2a69c57f6b5414
transactionHash: 0xa8b32f4214d54d893fbacbbbc6bf23e4802781c2cbd5b6b39891887b9429b5ab
OrderValidator successfully deployed at 0x32eecaf51dfea9618e9bc94e9fbfddb1bbdcba15
transactionHash: 0x049d5228f02b5bab8c04777d0cb6069d511577504802203c161da49edff0185a
DutchAuction successfully deployed at 0x7e3f4e1deb8d3a05d9d2da87d9521268d0ec3239
transactionHash: 0xc8f8a97723238560788604339410cb468e780a5f5cb644ca025e54ee403bbd3b
AssetProxyOwner successfully deployed at 0x04b5dadd2c0d6a261bfafbc964e0cac48585def3

Testing instructions

Types of changes

Checklist:

  • Prefix PR title with [WIP] if necessary.
  • Add tests to cover changes as needed.
  • Update documentation as needed.
  • Add new entries to the relevant CHANGELOG.jsons.

@coveralls
Copy link

coveralls commented Jan 21, 2019

Coverage Status

Coverage remained the same at 85.289% when pulling 6f090a2 on migrations/consolidate-owner-default into b5fd3c7 on development.

@dekz dekz requested a review from albrow January 21, 2019 04:56
@dekz dekz changed the title [WIP] Remove owner from migrations Remove owner from migrations Jan 21, 2019
@dekz dekz requested a review from fabioberger January 21, 2019 04:57
Copy link
Contributor

@fabioberger fabioberger left a comment

Choose a reason for hiding this comment

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

@dekz, is this a backward compatible change for existing snapshot users?

@dekz
Copy link
Member Author

dekz commented Jan 22, 2019

@fabioberger it results in the same output when from address is the first address in the mnemonic. Same contract addresses etc.

Those who specified a from address via the cli would not have had success in running the migrations.

Copy link
Contributor

@albrow albrow left a comment

Choose a reason for hiding this comment

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

@fabio may know more about how this changes affects different classes of users or interacts with other packages.

The code changes to the migrations package itself are simple enough and look good to me.

@@ -5,6 +5,14 @@
{
"note": "Upgrade the bignumber.js to v8.0.2",
"pr": 1517
},
{
"note": "Removed `owner` in Migrations. `txDefaults` is now a non-Partial type",
Copy link
Contributor

@albrow albrow Jan 22, 2019

Choose a reason for hiding this comment

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

txDefaults is now a non-Partial type

Optional: It's not very clear how this affects the API for callers. Should we just say something like "The from field is now required in the txDefaults argument for runMigrationsAsync"? That's what I would ultimately want to know if I were a developer using the migrations package as a library.

@dekz dekz merged commit da357f7 into development Jan 31, 2019
@dekz dekz deleted the migrations/consolidate-owner-default branch January 31, 2019 06:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants