Branded token and Gateway composer interact with unit tests. #99

0xsarvesh · 2019-02-18T12:31:25Z

Fixes #82 and #83

Added branded token contract and gateway composer contact interacts with unit tests.
Refer contract from here and here

…teract

schemar

I did not check the tests, the other comments should be addressed first.

In addition to in-line comments:

A lot of JSDoc is missing the {type} annotation.
The contract interacts should also have getters for the public variables.
It would have been nicer to have one PR per contract interact.

.travis.yml

schemar · 2019-02-20T12:04:48Z

lib/ContractInteract/BrandedToken.js

+    this.liftRestrictionRawTx = this.liftRestrictionRawTx.bind(this);
+    this.isUnrestricted = this.isUnrestricted.bind(this);
+    this.rejectStakeRequest = this.rejectStakeRequest.bind(this);
+    this.rejectStakeRequestRawTx = this.rejectStakeRequestRawTx.bind(this);


missing (from https://github.com/OpenSTFoundation/brandedtoken-contracts/blob/develop/contracts/BrandedToken.sol):

liftAllRestrictions

revokeStakeRequest

redeem

setSymbol

setName

transfer

transferFrom

convertToValueTokens

Intentionally skipped, as they are not used currently and of low priority for now.

Hm... Is there another ticket for them? How do we communicate the lack of methods to the consumer?

Refer #104
Note: There is a default way to access all the contract methods by contractInteract.contract.methods.xyz
Note should be added in the readme that for a missing method of contract interact use contractInteract.contract.methods.xyz

Is there another ticket for the getters of public variables? They are also missing from other ContractInteracts.

lib/ContractInteract/BrandedToken.js

utils/Utils.js

schemar · 2019-02-20T12:30:29Z

utils/Utils.js

+   * @returns {Promise} Promise object.
+   */
+  static async sendTransaction(tx, txOption) {
+    return new Promise(async (onResolve, onReject) => {


Is it really required to construct a new promise instead of using the one from tx.send()?

I am not sure, I have taken this from mosaic.js

It seems redundant to me 🤔

PR feedback changes for documentation Co-Authored-By: sarvesh-ost <[email protected]>

0xsarvesh · 2019-02-20T14:46:49Z

I did not check the tests, the other comments should be addressed first.

In addition to in-line comments:

A lot of JSDoc is missing the {type} annotation.

The contract interacts should also have getters for the public variables.

It would have been nicer to have one PR per contract interact.

I am bit surprise for the feedback changes in below files. Below files are directly taken from mosaic.js. Why these were not fixed there 😱

AssertAsync
Spy
Utils

abhayks1

Overall looks good to me 💯. Learned more on writing JS unit tests using sinon 🤓 .
I have requested few changes. I think considering repetitive samilar validations we can refactor it to validation library/util. We should discuss this and implement it if everyone agrees.

abhayks1 · 2019-02-18T15:10:48Z

lib/ContractInteract/BrandedToken.js

+   * @param {Object} web3 Web3 object.
+   * @param {string} address BrandedToken contract address.
+   */
+  constructor(web3, address) {


Should we call web3 => "originWeb3" as BT is deployed on origin chain?

In my opinion, the BrandedToken class is not concerned with what's origin or auxiliary. It only cares that it is deployed on a chain that it can access with a web3 object.

no, rather moving it to web3 from where it is specified (and ambivalent)

abhayks1 · 2019-02-21T12:06:51Z

lib/ContractInteract/BrandedToken.js

+   *                                  instance that has been deployed.
+   */
+  static async deploy(
+    web3,


unit test for deploy and deployRawTx method is missing 👁

abhayks1 · 2019-02-21T12:08:17Z

test/unit/BrandedToken/acceptStakeRawTx.js

+    Spy.assert(spyRawTx, 1, [[stakeRequestHash, r, s, v]]);
+    sinon.restore();
+  });
+  it('should throw an error when stakeRequestHash is invalid', async () => {


New line before it is missing 👀

abhayks1 · 2019-02-21T12:18:02Z

lib/ContractInteract/BrandedToken.js

+      const err = new TypeError('Invalid transaction options.');
+      return Promise.reject(err);
+    }
+    if (!Web3.utils.isAddress(txOptions.from)) {


Since the validations for typechecks are repetitive, I think we should create Validation library/util where we can refactor the code. Validation library/util can also be used in mosaic.js and openst.js also. @schemar What do you think?

That could help 👍

abhayks1 · 2019-02-21T12:21:10Z

lib/ContractInteract/GatewayComposer.js

+   * @param {Web3} web3 Web3 instance that points to origin.
+   * @param {string} address Gateway composer contract address.
+   */
+  constructor(web3, address) {


Should we rename web3 to originWeb3 as GatewayComposer is deployed on origin chain?

abhayks1 · 2019-02-21T12:29:23Z

lib/ContractInteract/GatewayComposer.js

+   * @param txOptions Transaction options.
+   *
+   * @returns {Promise<Object>} Promise that resolves to transaction receipt.
+   */


We should mention in documentation as dev note that "requestStake is called by staker after GC is approved for ValueToken."

abhayks1 · 2019-02-21T12:32:28Z

lib/ContractInteract/GatewayComposer.js

+      const err = new TypeError(`Invalid nonce: ${nonce}.`);
+      return Promise.reject(err);
+    }
+


Please add below validations:
"Gateway address can't be 0x000000..."
"Beneficiary address can't be 0x000000..."

Should we validate that:
"Converted stakeVT should be equal to mintBT"
There will be a geth call need for that. To avoid get call, we can have a simple conversion logic in javascript.

abhayks1 · 2019-02-21T12:39:30Z

lib/ContractInteract/BrandedToken.js

+    if (!(conversionRate > 0)) {
+      throw new TypeError(`Invalid conversion rate: ${conversionRate}. It should be greater than zero`);
+    }
+    if (!(conversionRateDecimals < 5)) {


The check should be "<=" because ConversionRateDecimals can be less/equal to 5.

conversionRateDecimals <= 5

abhayks1 · 2019-02-21T12:45:09Z

lib/ContractInteract/BrandedToken.js

+      return Promise.reject(err);
+    }
+
+    const mintedAmount = await this.convertToBrandedTokens(stakeAmount);


I believe requestStake interface should be aligned with BrandedToken.requestStake.
So mintAmount should not be calculated in this function. It causes an extra geth call and updates the interfaces. Staker can call convertToBrandedTokens to know the mint amount.

schemar · 2019-02-22T14:12:37Z

lib/ContractInteract/BrandedToken.js

+   * @param {Object} web3 Web3 object.
+   * @param {string} address BrandedToken contract address.
+   */
+  constructor(web3, address) {


In my opinion, the BrandedToken class is not concerned with what's origin or auxiliary. It only cares that it is deployed on a chain that it can access with a web3 object.

schemar · 2019-02-22T14:13:29Z

lib/ContractInteract/BrandedToken.js

+      const err = new TypeError('Invalid transaction options.');
+      return Promise.reject(err);
+    }
+    if (!Web3.utils.isAddress(txOptions.from)) {


That could help 👍

schemar · 2019-02-22T14:15:40Z

lib/ContractInteract/BrandedToken.js

+   * @param conversionRateDecimals The value to which
+   *                                conversionRateDecimals is set.
+   * @param organization Organization contract address.
+   *


I don't find @dev as a valid JSDoc annotation. It would also not be correct as this info is not intended for developers, but for consumers of the function and should therefore not be hidden behind @dev.
Why don't you explain the parameters where they're supposed to be explained?

schemar · 2019-02-22T14:18:35Z

lib/ContractInteract/BrandedToken.js

+      throw new TypeError(`Invalid conversion rate decimal: ${conversionRateDecimals}. It should be less than 5`);
+    }
+
+    const abiBinProvider = new AbiBinProvider();


Then pass it as an argument to the static method. Alternatively pass the bin.

schemar · 2019-02-22T14:21:59Z

lib/ContractInteract/BrandedToken.js

+  /**
+   * Raw tx for request stake.
+   *
+   * @param {string} stakeAmount Stake amount.


A lot of the methods could use better documentation. For example: the stake amount as a decimal or as an integer with the decimals included?

schemar · 2019-02-22T14:38:38Z

test/utils/Spy.js

+   *
+   * @param {Object} spy Spy object for a function.
+   * @param {number} callCount number of times the function was called.
+   * @param {Array} inputArgs Two dimensional array where each one


Suggested change

* @param {Array} inputArgs Two dimensional array where each one

* @param {Array[]} inputArgs Expected input arguments to each call. Array of calls that contains an array of call arguments of each call.

schemar · 2019-02-22T14:39:13Z

test/utils/Spy.js

+   * This compares values of two arrays index by index.
+   *
+   * @param {Array} actualArguments The First array which represents the actual
+   *                          value in the assertion.


Suggested change

* value in the assertion.

* values in the assertion.

schemar · 2019-02-22T14:45:19Z

test/utils/Spy.js

+   * @param {Array} expectedArguments The second array which represents the
+   *                                  expected value in the assertion.
+   */
+  static assertArguments(actualArguments, expectedArguments) {


You are mixing concepts here. assertArguments is actually nothing else than assert.deepEqual(), which is independent from arguments. The fact that the arrays passed to the method are arguments shouldn't concern a function that does array equality check. Anyways, it seems we don't need this method and can replace it with assert.deepEqual()?

schemar · 2019-02-22T14:46:49Z

utils/Utils.js

+
+/**
+ * This class includes the functions shared among various classes.
+


Suggested change

schemar · 2019-02-22T14:47:43Z

utils/Utils.js

+   * @returns {Promise} Promise object.
+   */
+  static async sendTransaction(tx, txOption) {
+    return new Promise(async (onResolve, onReject) => {


It seems redundant to me 🤔

benjaminbollen

LGTM

Sarvesh Jain added 7 commits February 18, 2019 11:35

Branded token contract interact

df51164

Merge remote-tracking branch 'upstream/develop' into branded_token_in…

4dc3bd8

…teract

Constructor tests

ddffa37

request_Stake tests

1c00e25

Unit tests for accept stake request

cdac015

Added tests for remaining methods, lifeRestriction

756d3a1

Added reject stake request

1b44297

0xsarvesh added the v0.10.0 label Feb 18, 2019

0xsarvesh self-assigned this Feb 18, 2019

0xsarvesh requested review from schemar and abhayks1 February 18, 2019 12:31

Sarvesh Jain added 6 commits February 18, 2019 18:03

Added integration test for travis

54cc372

Fixed typo in documentation

56142bd

Added Gateway composer class'

54411c9

Gateway composer request stake

621c488

Merge remote-tracking branch 'upstream/develop' into branded_token_in…

56ffde7

…teract

Added revert and revoke stake

2114826

0xsarvesh changed the title ~~Branded token interact with unit tests.~~ Branded token and Gateway composer interact with unit tests. Feb 19, 2019

0xsarvesh mentioned this pull request Feb 20, 2019

Utility branded token contract interact #103

Merged

Binding methods of contract interacts to this

5e62ba5

schemar suggested changes Feb 20, 2019

View reviewed changes

Apply suggestions from code review

8d44367

PR feedback changes for documentation Co-Authored-By: sarvesh-ost <[email protected]>

Sarvesh Jain added 4 commits February 21, 2019 14:10

PR feedback changes: enchanced documentation

630df14

Fixed typos

053d514

Added tests for convertToBrandedTokens

516acd7

Added types to params in the documentation

0e26e87

abhayks1 suggested changes Feb 21, 2019

View reviewed changes

0xsarvesh mentioned this pull request Feb 22, 2019

Facilitator and staker now uses contract interact #105

Merged

schemar self-requested a review February 22, 2019 10:50

schemar reviewed Feb 22, 2019

View reviewed changes

benjaminbollen approved these changes Feb 22, 2019

View reviewed changes

benjaminbollen mentioned this pull request Feb 22, 2019

Follow up with unresolved review comments #106

Closed

benjaminbollen merged commit 5806895 into OpenST:develop Feb 22, 2019

hobofan mentioned this pull request Feb 25, 2019

Consistently use txOptions #96

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Branded token and Gateway composer interact with unit tests. #99

Branded token and Gateway composer interact with unit tests. #99

0xsarvesh commented Feb 18, 2019 •

edited

Loading

schemar left a comment

schemar Feb 20, 2019

0xsarvesh Feb 20, 2019

schemar Feb 20, 2019

0xsarvesh Feb 21, 2019 •

edited

Loading

schemar Feb 22, 2019

schemar Feb 20, 2019

0xsarvesh Feb 21, 2019

schemar Feb 22, 2019

0xsarvesh commented Feb 20, 2019

abhayks1 left a comment

abhayks1 Feb 18, 2019

schemar Feb 22, 2019

benjaminbollen Feb 22, 2019

abhayks1 Feb 21, 2019

abhayks1 Feb 21, 2019

abhayks1 Feb 21, 2019 •

edited

Loading

schemar Feb 22, 2019

abhayks1 Feb 21, 2019

abhayks1 Feb 21, 2019

abhayks1 Feb 21, 2019

abhayks1 Feb 21, 2019

abhayks1 Feb 21, 2019 •

edited

Loading

schemar Feb 22, 2019

schemar Feb 22, 2019

schemar Feb 22, 2019

schemar Feb 22, 2019

schemar Feb 22, 2019

schemar Feb 22, 2019

schemar Feb 22, 2019

schemar Feb 22, 2019

schemar Feb 22, 2019

schemar Feb 22, 2019

benjaminbollen left a comment

	* @param {Array} inputArgs Two dimensional array where each one
	* @param {Array[]} inputArgs Expected input arguments to each call. Array of calls that contains an array of call arguments of each call.


		/**
		* This class includes the functions shared among various classes.

Branded token and Gateway composer interact with unit tests. #99

Branded token and Gateway composer interact with unit tests. #99

Conversation

0xsarvesh commented Feb 18, 2019 • edited Loading

schemar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xsarvesh Feb 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xsarvesh commented Feb 20, 2019

abhayks1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhayks1 Feb 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhayks1 Feb 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjaminbollen left a comment

Choose a reason for hiding this comment

0xsarvesh commented Feb 18, 2019 •

edited

Loading

0xsarvesh Feb 21, 2019 •

edited

Loading

abhayks1 Feb 21, 2019 •

edited

Loading

abhayks1 Feb 21, 2019 •

edited

Loading