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.

Sign up for GitHub

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

Jump to bottom

Some issues and threads #358

Merged
merged 31 commits into from
Apr 30, 2019
Merged

Some issues and threads #358

Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
8844364
Updating npm to 10.15
marcelomorgado Apr 27, 2019
ad3ee7a
Updating lerna and truffle
marcelomorgado Apr 27, 2019
a583693
Updating Circle CI config
marcelomorgado Apr 27, 2019
8431737
Updating package.json md5sum
marcelomorgado Apr 27, 2019
68a4e34
Downgrade truffle-hdwallet-provider
marcelomorgado Apr 27, 2019
23c89cb
Downgrading lerna
marcelomorgado Apr 27, 2019
3d439fe
Pinning lerna to 3.13.2
marcelomorgado Apr 27, 2019
8a05fad
Increasing timeout
marcelomorgado Apr 27, 2019
291bc2a
lockfile
marcelomorgado Apr 27, 2019
1109054
Downgrading truffle hdwallet
marcelomorgado Apr 27, 2019
e7247af
test
marcelomorgado Apr 27, 2019
f99693a
Updating package.json
marcelomorgado Apr 27, 2019
aba4372
Test fixes
marcelomorgado Apr 27, 2019
e8b4674
CI without cache
marcelomorgado Apr 27, 2019
c8cb4f1
Non-determinism
marcelomorgado Apr 27, 2019
1d69551
Non-determinism
marcelomorgado Apr 27, 2019
48402b5
Contract.test
marcelomorgado Apr 29, 2019
2c4e7c5
ERC721Full.test
marcelomorgado Apr 29, 2019
9cf891f
no-timeout
marcelomorgado Apr 29, 2019
5114a28
timeouts
marcelomorgado Apr 29, 2019
f936c9c
Action without tx warn
marcelomorgado Apr 29, 2019
9939b75
Timeouts
marcelomorgado Apr 29, 2019
907291a
Adjustments
marcelomorgado Apr 29, 2019
4a117d3
Cache
marcelomorgado Apr 29, 2019
a1eabe4
Disabling CI cache
marcelomorgado Apr 29, 2019
9e714a0
Skip non-determinism
marcelomorgado Apr 29, 2019
bf3d677
Updating truffle-hdwallet-provider
marcelomorgado Apr 29, 2019
e17318a
Listening before sending test case
marcelomorgado Apr 30, 2019
1f1e5f6
Review request
marcelomorgado Apr 30, 2019
b0fe2ff
Review request
marcelomorgado Apr 30, 2019
666719d
Review request
marcelomorgado Apr 30, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ jobs:
build:
docker:
# specify the version you desire here
- image: circleci/node:10.13
- image: circleci/node:10.15

# Specify service dependencies here if necessary
# CircleCI maintains a library of pre-built images
Expand All @@ -32,20 +32,20 @@ jobs:
command: "greenkeeper-lockfile-update"

# Download and cache dependencies
- restore_cache:
keys:
- dependency-cache-{{ checksum "package.json" }}
# fallback to using the latest cache if no exact match is found
- dependency-cache-
#- restore_cache:
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, are you sure we want to disable the cache?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, just saw this comment:

Disabling CI node_modules caching to solve failure after truffle-hdwallet-provider upgrade. (Refs: https://circleci.com/gh/tasitlabs/TasitSDK/593)

Copy link
Member

Choose a reason for hiding this comment

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

Should we consider downgrading that package instead? Will we be able to re-enable caching at any point?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can downgrade truffle-hdwallet-provider package or try to re-enable in the future.

Copy link
Member

Choose a reason for hiding this comment

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

What in the error message indicated to you that it was realted to truffle-hdwallet-provider?

> npx lerna bootstrap --hoist

lerna notice cli v3.13.4
lerna info ci enabled
lerna info Bootstrapping 6 packages
lerna info Installing external dependencies
lerna info hoist Installing hoisted dependencies into root
lerna info hoist Pruning hoisted dependencies
lerna info hoist Finished pruning hoisted dependencies
lerna ERR! npm install --no-save exited 1 in 'root'
lerna ERR! npm install --no-save stderr:
npm ERR! path /home/circleci/tasit-sdk/node_modules/web3-providers-ws/node_modules/websocket
npm ERR! code EISGIT
npm ERR! git /home/circleci/tasit-sdk/node_modules/web3-providers-ws/node_modules/websocket: Appears to be a git repo or submodule.
npm ERR! git     /home/circleci/tasit-sdk/node_modules/web3-providers-ws/node_modules/websocket
npm ERR! git Refusing to remove it. Update manually,
npm ERR! git or move it out of the way first.

Copy link
Member

@pcowgill pcowgill Apr 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

The results you shared are convincing - I think lerna's docs are wrong, then. Because it says that it hoists "shared" dependencies.

Copy link
Member

Choose a reason for hiding this comment

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

I made this issue on the lerna repo lerna/lerna#2062

Copy link
Contributor Author

Choose a reason for hiding this comment

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

packages/tasit-action/node_modules:
tasit-account tasit-contracts

This one is surprising then, right?

I didn't get it. tasit-action is using tasit-contracts to resolve contracts ABI/address and tasit-account as devDependency to access createFromPrivateKey helper.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue to enable cache in future: #359

Copy link
Member

Choose a reason for hiding this comment

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

packages/tasit-action/node_modules:
tasit-account tasit-contracts

This one is surprising then, right?

I didn't get it. tasit-action is using tasit-contracts to resolve contracts ABI/address and tasit-account as devDependency to access createFromPrivateKey helper.

Yep. So why aren't they hoisted like all of our other deps?

# keys:
# - dependency-cache-{{ checksum "package.json" }}
# fallback to using the latest cache if no exact match is found
Copy link
Member

Choose a reason for hiding this comment

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

Seems like we definitely won't want to enable this

# - dependency-cache-

- run:
name: Bootstrapping packages
command: npm run bootstrap

- save_cache:
paths:
- ./node_modules
key: dependency-cache-{{ checksum "package.json" }}
#- save_cache:
# paths:
# - ./node_modules
# key: dependency-cache-{{ checksum "package.json" }}
pcowgill marked this conversation as resolved.
Show resolved Hide resolved

- run:
name: Running test suite
Expand Down
2 changes: 1 addition & 1 deletion .nvmrc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
10.13
10.15
2,752 changes: 2,319 additions & 433 deletions package-lock.json
View file
Open in desktop

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
"devDependencies": {
"@babel/preset-env": "^7.4.1",
"ganache-cli": "^6.4.3",
"lerna": "^3.13.2",
"lerna": "^3.13.4",
"prettier": "^1.17.0",
"prettier-plugin-solidity": "^1.0.0-alpha.22"
},
Expand Down Expand Up @@ -41,7 +41,7 @@
},
"homepage": "https://github.com/tasitlabs/TasitSDK#readme",
"engines": {
"node": ">=10.13.0",
"node": ">=10.15.0",
"npm": ">=6.4.1"
}
}
2 changes: 1 addition & 1 deletion packages/tasit-action/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
},
"scripts": {
"prepare": "rm -rf dist/* && npx babel src --out-dir dist --ignore src/*.test.js,src/**/*.test.js",
"test": "npm run lint && npx mocha \"src/**/*.test.js\" --recursive --require @babel/register --file src/testHelpers/mochaSetup.js",
"test": "npm run lint && npx mocha \"src/**/*.test.js\" --recursive --require @babel/register --file src/testHelpers/mochaSetup.js --timeout 8000",
"lint": "npx prettier src/* --write"
},
"bugs": {
Expand Down
2 changes: 1 addition & 1 deletion packages/tasit-action/src/config/default.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ const development = {
},
},
events: {
timeout: 2000,
timeout: 4000,
},
};

Expand Down
4 changes: 4 additions & 0 deletions packages/tasit-action/src/contract/Action.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,10 @@ export class Action extends Subscription {
const baseEthersListener = async blockNumber => {
try {
const tx = await this.#tx;
if (!tx) {
console.log(`The action wasn't sent yet.`);
Copy link
Member

Choose a reason for hiding this comment

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

How does this log statement in a dependency (our own) get handled with our removing log approach in the Tasit apps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I should have used the warn instead.
The console removal plugin used on tasit removes all console.* from code.
Not sure about the best way to deal with warn, not sure if make sense use error event for that. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Oh yes, that's a good point that this should be warn.

If this is something that would prevent the tx from going through, then passing this back to the caller as an error so they can retry the tx and update the UI accordingly probably makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, just thought again about it: It's happening when an action starts to be listening before send it (the tx field is assigned on the send function). I think that worths a test case, and maybe is a situation to be ignored?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that's worth a test case

I'm not sure if it's a situation to be ignored, though? Why would it be okay to ignore it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've wrote this test case:

    it("should be able to listen to an event before sending", async () => {
      const confirmationListener = sinon.fake(async message => {
        action.off("confirmation");
      });

      const errorListener = sinon.fake();

      action = sampleContract.setValue(rand);
      action.on("error", errorListener);
      action.on("confirmation", confirmationListener);

      await mineBlocks(provider, 2);

      await action.send();
      await action.waitForOneConfirmation();

      await mineBlocks(provider, 2);

      expect(confirmationListener.callCount).to.equal(1);
      expect(errorListener.called).to.be.false;
    });

That way, warn is being printed for each new block mined, I'm not sure if it's desirable.
Since:

  1. It's expected behavior;
  2. I've improved the signAndSend function (that assigns the this.#tx field) to guarantee that it'll emit an error event if some error occurs;
    Maybe we could remove this warn.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this gate merging?

I don't think so.

Copy link
Member

Choose a reason for hiding this comment

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

Ahh ok - I forgot that the action not being sent yet was something we would want to happen sometimes.

Based on that, what I said earlier:

If this is something that would prevent the tx from going through, then passing this back to the caller as an error so they can retry the tx and update the UI accordingly probably makes sense.

...didn't make sense. So I'm glad you wrote this test as a way of clarifying how we would run into this scenario. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

That way, warn is being printed for each new block mined, I'm not sure if it's desirable.

Yep, this is an argument for moving it back to INFO level

Copy link
Member

Choose a reason for hiding this comment

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

  1. I've improved the signAndSend function (that assigns the this.#tx field) to guarantee that it'll emit an error event if some error occurs;

Oh okay, great!

Maybe we could remove this warn.

Oh remove it, not just change it to an INFO log? Maybe if we INFO log when sending and INFO log when listening we could remove it here.

return;
}

const receipt = await this.#provider.getTransactionReceipt(tx.hash);

Expand Down
Loading