Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Fix #6540 #6556

Merged
merged 2 commits into from
Sep 21, 2017
Merged

Fix #6540 #6556

merged 2 commits into from
Sep 21, 2017

Conversation

travs
Copy link
Contributor

@travs travs commented Sep 19, 2017

Skips the checkRequest call if result of postTransaction is a tx ID.
If there is a better way to check whether result is a tx ID than looking at its length, let's do that.

Made a similar PR over here, but I can't even get the module installed from that repo, and the latest changes are not as new as those in this repo, so it looks like this one makes more sense to merge to.

@parity-cla-bot
Copy link

It looks like @travs signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@travs
Copy link
Contributor Author

travs commented Sep 19, 2017

Not sure why I keep getting this failing build.
I really don't think it is to do with my change.
I'm branching from master; is this the wrong thing to do?

@jacogr
Copy link
Contributor

jacogr commented Sep 19, 2017

Failures fixed in #6553

@travs
Copy link
Contributor Author

travs commented Sep 19, 2017

@jacogr
Copy that. Will rebase this guy when #6553 merges

@rphmeier rphmeier added A0-pleasereview 🤓 Pull request needs code review. M7-ui labels Sep 19, 2017
@gavofyork
Copy link
Contributor

done

@5chdn 5chdn added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 20, 2017
- just skip the checkRequest call if result of postTransaction is a tx ID
@travs
Copy link
Contributor Author

travs commented Sep 20, 2017

Rebased and ready to merge (if it passes 🤞 )

return this._pollCheckRequest(requestId);
.then((result) => {
if (result.length !== 66) {
statecb(null, { state: 'checkRequest', result });
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did we lose the requestId naming? This is is that the holder of the statecb checks the requestId field which does not exist anywhere anymore.

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 yeah that's my mistake, one sec I will revise

Copy link
Contributor Author

Choose a reason for hiding this comment

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

revised 9deac99

@5chdn 5chdn added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Sep 20, 2017
@jacogr
Copy link
Contributor

jacogr commented Sep 20, 2017

LGTM

As for the parity.js repo, it will be removed in favour of the js-{abi, api} repos. (I do keep those updated from master, at this point they are direct match* for what is found there, in the short term they will become the only source of truth.)

  • Almost a direct match. Some changes have been made to allow them to be used as-is from both the web and node environments without a compilation step. (Basically makes them easier to embed/use)

@jacogr jacogr added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 20, 2017
@travs
Copy link
Contributor Author

travs commented Sep 20, 2017

@jacogr Ok great. I want to use js-api from node so I can just install via the instructions in the readme here?

@jacogr
Copy link
Contributor

jacogr commented Sep 20, 2017

@travs Will update the instructions.

But basically, not published to npm (yet, we are busy working through that with CI, will be in the next week or so), so for now would just need to have a github package.json entry: (Also the way we use it atm with out next-generation UI until the package is published)

{
  ...
  "dependencies" {
    "@parity/api": "paritytech/js-api"
    ...
  }
  ...
}

Then just use it as you would the normal parity.js, i.e.

const ParityApi = require('@parity/api");

// Only difference here to the previous version:
// Transport changed to Provider to align with web3/metamask/next-gen
// (Could also push in the web3 provider once they support the new .send() interface shortly
// PS: Could still use Transport as per normal, it will just show a warning message
const parity = new ParityApi(new ParityApi.Provider.Http('...'));

@gavofyork gavofyork merged commit 9e0d2c1 into openethereum:master Sep 21, 2017
travs pushed a commit to travs/js-api that referenced this pull request Sep 21, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants