feat(mixin-ownable): add Ownable mixin and tests #1305

davemneo · 2019-04-10T02:33:13Z

Overview

This class provides a means to assign an address as the owner of a contract. Extending this class and adding this.ownerOnly(); to the beginning of all public functions will throw an error anytime an address other than the primary makes requests.

API

Public

owner() - returns the current owner address or undefined.

Manage contract Ownership with...

renounceOwnership() - Allows only the owner to contract to become ownerless, this will cause all ownerOnly() calls to fail, this is not reversible. Note: the initialOwner will maintain still contain the initial address.
transferOwnership() - Only the owner may transfer ownership to a new

Internal

ownerOrThrow() - returns the current owner address or throws an error.
onlyOwner() - if the owner address did not call this contract an error is thrown.

Published Events:

tokensPurchased with purchaser, beneficiary and amount of tokens bought.

Configuration:

Be sure that any contract extending this Ownable mix-in sets the initialOwner thusly:

export class MyContract extends Ownable(SmartContract) {
  public constructor(protected readonly initialOwner = Deploy.senderAddress) {
    super();
  }
  // .... awesome smart contract code ... 
}

Tests

yarn jest TestOwnable

codecov · 2019-04-10T03:25:18Z

Codecov Report

Merging #1305 into master will increase coverage by 0.05%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master   #1305      +/-   ##
=========================================
+ Coverage   64.74%   64.8%   +0.05%     
=========================================
  Files        1529    1529              
  Lines       40396   40396              
  Branches     3941    3941              
=========================================
+ Hits        26156   26179      +23     
+ Misses      14240   14217      -23

Flag	Coverage Δ
#e2e	`60.41% <ø> (-0.08%)`	⬇️
#unit	`62.38% <ø> (+0.13%)`	⬆️

Impacted Files	Coverage Δ
...eo-one-node-protocol/src/payload/HeadersPayload.ts	`21.42% <0%> (-35.72%)`	⬇️
...-one-node-protocol/src/payload/GetBlocksPayload.ts	`64.7% <0%> (-35.3%)`	⬇️
.../payload/message/PrepareRequestConsensusMessage.ts	`34.37% <0%> (-34.38%)`	⬇️
...core/src/transaction/attribute/UInt256Attribute.ts	`57.57% <0%> (-6.07%)`	⬇️
...neo-one-node-http-rpc/src/middleware/checkReady.ts	`91.17% <0%> (-2.95%)`	⬇️
...erver-plugin-network/src/node/NEOONENodeAdapter.ts	`80.59% <0%> (-1.5%)`	⬇️
packages/neo-one-ts-utils/src/type_.ts	`71.29% <0%> (-0.31%)`	⬇️
...eo-one-node-blockchain/src/WriteBatchBlockchain.ts	`86.85% <0%> (+0.3%)`	⬆️
.../neo-one-client-core/src/sc/createSmartContract.ts	`96.58% <0%> (+0.85%)`	⬆️
.../neo-one-node-blockchain/src/wrapExecuteScripts.ts	`75% <0%> (+12.5%)`	⬆️
... and 4 more

codecov · 2019-04-10T03:25:19Z

Codecov Report

Merging #1305 into master will increase coverage by 2.56%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1305      +/-   ##
==========================================
+ Coverage   62.23%   64.79%   +2.56%     
==========================================
  Files        1529     1529              
  Lines       40393    40407      +14     
  Branches     3940     3943       +3     
==========================================
+ Hits        25137    26183    +1046     
+ Misses      15256    14224    -1032

Flag	Coverage Δ
#e2e	`60.29% <ø> (-39.71%)`	⬇️
#unit	`62.38% <ø> (+0.15%)`	⬆️

Impacted Files	Coverage Δ
packages/neo-one-website/src/polyfill.ts	`0% <0%> (ø)`	⬆️
...eo-one-node-blockchain/src/WriteBatchBlockchain.ts	`86.85% <0%> (+0.3%)`	⬆️
packages/neo-one-node-vm/src/opcodes.ts	`84.34% <0%> (+0.5%)`	⬆️
packages/neo-one-node-vm/src/errors.ts	`64.1% <0%> (+0.85%)`	⬆️
packages/neo-one-node-vm/src/execute.ts	`82.47% <0%> (+1.03%)`	⬆️
...ne-client-core/src/provider/JSONRPCHTTPProvider.ts	`93.1% <0%> (+1.14%)`	⬆️
...o-one-node-core/src/transaction/TransactionBase.ts	`86.98% <0%> (+1.77%)`	⬆️
.../neo-one-client-core/src/sc/createSmartContract.ts	`96.58% <0%> (+2.56%)`	⬆️
...ient-switch/src/common/createConsoleLogMessages.ts	`90.9% <0%> (+2.59%)`	⬆️
...ckages/neo-one-node-core/src/utils/BinaryReader.ts	`83.01% <0%> (+5.66%)`	⬆️
... and 82 more

packages/neo-one-smart-contract-lib/src/ownership/Ownable.ts

packages/neo-one-smart-contract-lib/src/index.ts

dicarlo2

overall lgtm

packages/neo-one-smart-contract-lib/src/ownership/Ownable.ts

tsconfig.json

packages/neo-one-smart-contract-lib/src/__tests__/ownership/TestOwnableContract.test.ts

dicarlo2

LG, just two last comments.

packages/neo-one-smart-contract-lib/src/__tests__/ownership/TestOwnableContract.test.ts

packages/neo-one-smart-contract-lib/src/ownership/Ownable.ts

dicarlo2

One last comment!

dicarlo2 · 2019-04-19T07:24:20Z

packages/neo-one-smart-contract-lib/src/ownership/Ownable.ts

@@ -0,0 +1,57 @@
+import { Address, SmartContract, createEventNotifier } from '@neo-one/smart-contract';
+
+const notifyTransferOwnership = createEventNotifier<Address | undefined, Address>('transfer_ownership', 'from', 'to');


the from Address can never be undefined, no?

@dicarlo2 True, now, after modifying transferOwnership to use ownerOrThrow otherwise, the mutableOwner as passed is possibly undefined.

Which path should these functions go? extra cpu cycles, or fewer with that optional undefined?

Answer discussed @ standup, going with "Easy to read".

dicarlo2 · 2019-04-19T07:25:47Z

packages/neo-one-smart-contract-lib/src/ownership/Ownable.ts

+    }
+
+    protected onlyOwner() {
+      if (this.mutableOwner !== undefined && !Address.isCaller(this.mutableOwner)) {


This should be using this.ownerOrThrow no? Seems like a bug - if there's no owner then this should throw I think.

@dicarlo2 "bug" might be a little harsh, I'd say mine is between 4 and 12 operations more efficient and achieves same goal. I can switch it to ownerOrThrow() if you still like.

I think he is suggesting something like:

protected onlyOwner() { if (!Address.isCaller(this.ownerOrThrow())) { throw new Error('not owner'); } }

(edit because assignment)

(because if this.mutableOwner is undefined then this will pass always)

@danwbyrne yes, that was my glitch the !==undefined. But: re discussion during standup, we are favoring code readability over scrutinizing nominal amounts of gas for extra stack operations. Change will be made, thank you!

danwbyrne

hmmm, what is the order that these PRs need to go in? I'm starting to get lost by them :P

davemneo · 2019-04-20T21:37:30Z

@danwbyrne

what is the order that these PRs need to go in? I'm starting to get lost by them :P

This one first.

danwbyrne

I see now this is the first one that needs to go in :)

danwbyrne · 2019-04-20T21:41:08Z

packages/neo-one-smart-contract-lib/src/ownership/Ownable.ts

+    }
+
+    protected onlyOwner() {
+      if (this.mutableOwner !== undefined && !Address.isCaller(this.mutableOwner)) {


I think he is suggesting something like:

protected onlyOwner() { if (!Address.isCaller(this.ownerOrThrow())) { throw new Error('not owner'); } }

(edit because assignment)

danwbyrne · 2019-04-20T21:43:34Z

packages/neo-one-smart-contract-lib/src/ownership/Ownable.ts

+    }
+
+    protected onlyOwner() {
+      if (this.mutableOwner !== undefined && !Address.isCaller(this.mutableOwner)) {


(because if this.mutableOwner is undefined then this will pass always)

danwbyrne

just fix the deepscan

davemneo · 2019-04-23T02:30:31Z

just fix the deepscan

@danwbyrne Unrelated

dicarlo2

lgtm.

The deepscan issues are odd, I wonder why they showed up on this PR.

dicarlo2 · 2019-04-23T10:21:34Z

bors r+

1305: feat(mixin-ownable): add Ownable mixin and tests r=dicarlo2 a=davemneo # Overview This class provides a means to assign an address as the owner of a contract. Extending this class and adding ` this.ownerOnly(); ` to the beginning of all public functions will throw an error anytime an address other than the primary makes requests. ## API ### Public `owner()` - returns the current owner address or undefined. ##### Manage contract Ownership with... `renounceOwnership()` - Allows only the owner to contract to become ownerless, this will cause all `ownerOnly()` calls to fail, this is not reversible. Note: the `initialOwner` will maintain still contain the initial address. `transferOwnership()` - Only the owner may transfer ownership to a new ### Internal `ownerOrThrow()` - returns the current owner address or throws an error. `onlyOwner()` - if the owner address did not call this contract an error is thrown. ### Published Events: `tokensPurchased` with purchaser, beneficiary and amount of tokens bought. ## Configuration: Be sure that any contract extending this `Ownable` mix-in sets the `initialOwner` thusly: ``` typescript: export class MyContract extends Ownable(SmartContract) { public constructor(protected readonly initialOwner = Deploy.senderAddress) { super(); } // .... awesome smart contract code ... } ``` ## Tests yarn jest TestOwnable Co-authored-by: Dave M <david.mathiesen@neotracker.io>

bors · 2019-04-23T10:46:56Z

Build succeeded

davemneo changed the title ~~feat(mixin-ownable): add Ownable mixin and tests suite~~ feat(mixin-ownable): add Ownable mixin and test suite Apr 10, 2019

davemneo changed the title ~~feat(mixin-ownable): add Ownable mixin and test suite~~ feat(mixin-ownable): add Ownable mixin and tests Apr 10, 2019

davemneo force-pushed the contract-mixins-ownable branch from da25ec6 to 2e5d552 Compare April 10, 2019 03:25

davemneo force-pushed the contract-mixins-ownable branch from 2e5d552 to 765fb3e Compare April 10, 2019 03:28

davemneo requested a review from afragapane April 10, 2019 16:59

dicarlo2 requested changes Apr 10, 2019

View reviewed changes

dicarlo2 reviewed Apr 10, 2019

View reviewed changes

packages/neo-one-smart-contract-lib/src/index.ts Outdated Show resolved Hide resolved

davemneo force-pushed the contract-mixins-ownable branch 7 times, most recently from 80a5837 to 6c69293 Compare April 12, 2019 04:16

davemneo added the ready for review label Apr 12, 2019

davemneo force-pushed the contract-mixins-ownable branch from 6c69293 to 2669607 Compare April 12, 2019 14:36

dicarlo2 requested changes Apr 13, 2019

View reviewed changes

davemneo force-pushed the contract-mixins-ownable branch 4 times, most recently from 202cf86 to 9f7469d Compare April 17, 2019 05:02

dicarlo2 requested changes Apr 17, 2019

View reviewed changes

packages/neo-one-smart-contract-lib/src/__tests__/ownership/TestOwnableContract.test.ts Outdated Show resolved Hide resolved

packages/neo-one-smart-contract-lib/src/ownership/Ownable.ts Outdated Show resolved Hide resolved

davemneo force-pushed the contract-mixins-ownable branch 3 times, most recently from d809969 to 7c47d5b Compare April 18, 2019 17:29

davemneo added FINAL REVIEW and removed ready for review labels Apr 18, 2019

davemneo force-pushed the contract-mixins-ownable branch from 7c47d5b to a747bc6 Compare April 18, 2019 19:08

dicarlo2 requested changes Apr 18, 2019

View reviewed changes

packages/neo-one-smart-contract-lib/src/ownership/Ownable.ts Outdated Show resolved Hide resolved

packages/neo-one-smart-contract-lib/src/ownership/Ownable.ts Outdated Show resolved Hide resolved

packages/neo-one-smart-contract-lib/src/ownership/Ownable.ts Outdated Show resolved Hide resolved

davemneo force-pushed the contract-mixins-ownable branch from a747bc6 to a704306 Compare April 19, 2019 05:53

dicarlo2 requested changes Apr 19, 2019

View reviewed changes

dicarlo2 reviewed Apr 19, 2019

View reviewed changes

danwbyrne suggested changes Apr 20, 2019

View reviewed changes

feat(mixin-ownable): add Ownable mixin and test

6f55ea7

davemneo force-pushed the contract-mixins-ownable branch from a704306 to 6f55ea7 Compare April 22, 2019 18:32

danwbyrne approved these changes Apr 22, 2019

View reviewed changes

dicarlo2 approved these changes Apr 23, 2019

View reviewed changes

bors bot merged commit 6f55ea7 into neo-one-suite:master Apr 23, 2019

davemneo self-assigned this Apr 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(mixin-ownable): add Ownable mixin and tests #1305

feat(mixin-ownable): add Ownable mixin and tests #1305

davemneo commented Apr 10, 2019 •

edited

codecov bot commented Apr 10, 2019

codecov bot commented Apr 10, 2019 •

edited

dicarlo2 left a comment

dicarlo2 left a comment •

edited

dicarlo2 left a comment

dicarlo2 Apr 19, 2019

davemneo Apr 22, 2019 •

edited

davemneo Apr 22, 2019

dicarlo2 Apr 19, 2019

davemneo Apr 20, 2019 •

edited

danwbyrne Apr 20, 2019

danwbyrne Apr 20, 2019

davemneo Apr 22, 2019 •

edited

danwbyrne left a comment •

edited

davemneo commented Apr 20, 2019 •

edited

danwbyrne left a comment

danwbyrne Apr 20, 2019

danwbyrne Apr 20, 2019

danwbyrne left a comment

davemneo commented Apr 23, 2019

dicarlo2 left a comment

dicarlo2 commented Apr 23, 2019

bors bot commented Apr 23, 2019

		@@ -0,0 +1,57 @@
		import { Address, SmartContract, createEventNotifier } from '@neo-one/smart-contract';

		const notifyTransferOwnership = createEventNotifier<Address \| undefined, Address>('transfer_ownership', 'from', 'to');

feat(mixin-ownable): add Ownable mixin and tests #1305

feat(mixin-ownable): add Ownable mixin and tests #1305

Conversation

davemneo commented Apr 10, 2019 • edited

Overview

API

Public

Manage contract Ownership with...

Internal

Published Events:

Configuration:

Tests

codecov bot commented Apr 10, 2019

Codecov Report

codecov bot commented Apr 10, 2019 • edited

Codecov Report

dicarlo2 left a comment

Choose a reason for hiding this comment

dicarlo2 left a comment • edited

Choose a reason for hiding this comment

dicarlo2 left a comment

Choose a reason for hiding this comment

dicarlo2 Apr 19, 2019

Choose a reason for hiding this comment

davemneo Apr 22, 2019 • edited

Choose a reason for hiding this comment

davemneo Apr 22, 2019

Choose a reason for hiding this comment

dicarlo2 Apr 19, 2019

Choose a reason for hiding this comment

davemneo Apr 20, 2019 • edited

Choose a reason for hiding this comment

danwbyrne Apr 20, 2019

Choose a reason for hiding this comment

danwbyrne Apr 20, 2019

Choose a reason for hiding this comment

davemneo Apr 22, 2019 • edited

Choose a reason for hiding this comment

danwbyrne left a comment • edited

Choose a reason for hiding this comment

davemneo commented Apr 20, 2019 • edited

danwbyrne left a comment

Choose a reason for hiding this comment

danwbyrne Apr 20, 2019

Choose a reason for hiding this comment

danwbyrne Apr 20, 2019

Choose a reason for hiding this comment

danwbyrne left a comment

Choose a reason for hiding this comment

davemneo commented Apr 23, 2019

dicarlo2 left a comment

Choose a reason for hiding this comment

dicarlo2 commented Apr 23, 2019

bors bot commented Apr 23, 2019

Build succeeded

davemneo commented Apr 10, 2019 •

edited

codecov bot commented Apr 10, 2019 •

edited

dicarlo2 left a comment •

edited

davemneo Apr 22, 2019 •

edited

davemneo Apr 20, 2019 •

edited

davemneo Apr 22, 2019 •

edited

danwbyrne left a comment •

edited

davemneo commented Apr 20, 2019 •

edited