Sparrow dom/tranaction hash #546

sparrowDom · 2018-09-25T15:14:43Z

Checklist:

Code contains relevant tests for the problem you are solving
Ensure all new and existing tests pass
Update any relevant READMEs and docs

Description:

**Connected PR: ** OriginProtocol/origin#577

franckc · 2018-09-25T16:10:25Z

Thanks @sparrowDom for working on this !
I can review once the discussion regarding approach to take on DApp side is settled a bit more... regardinhttps://github.com/OriginProtocol/origin/pull/577

sparrowDom · 2018-09-25T16:30:20Z

@franckc yup I guess this is currently more in a suggestion/proof of concept stage.

tyleryasaka

I'm ok with this for now, but this makes me think we definitely need to have an options object for every method in the origin-js API. 2 arguments that would be placed in the options object are confirmationCallback and transactionHashCallback.

sparrowDom · 2018-09-26T08:26:19Z

@tyleryasaka agreed, have created an issue for unification of the api: #555

sparrowDom · 2018-09-26T21:31:38Z

@tyleryasaka can you please take another look. Have added a more general options approach that could be used in unification api step that we will do some time later.

franckc

Thanks for working on this. Couple minor comments inline.

Also, could you add a unit test for the new functionality you are adding ? probably under test/resource_users.test.js

franckc · 2018-09-26T22:04:20Z

src/services/contract-service.js

+      confirmationCallback(confirmations, {
+        transactionHash: transactionInfo.hash
+      })
+      // do checks untill 5 block confirmations


typo: "until"
nit: let's define 5 as a constant. Like NUM_CONFIRMATIONS

franckc · 2018-09-26T22:04:56Z

src/resources/users.js

@@ -5,8 +5,8 @@ class Users {
    this.resolver = new UsersResolver({ contractService, ipfsService })
  }

-  async set({ profile, attestations = [] }) {
-    return this.resolver.set({ profile, attestations })
+  async set({ profile, attestations = [], options = {}}) {


nit: can we document as a comment what options are available ?

sparrowDom · 2018-09-26T22:48:11Z

@franckc thanks for reminding me about tests. Inline comments are addressed and a test added.

franckc

Thanks for integrating my feedback and adding a unit test. One comment inline regarding test.

Sorry for being so picky, this is a pretty critical area of our codebase and I want to make sure we are really thorough...

franckc · 2018-09-27T04:32:02Z

test/resource_users.test.js

+      })
+
+      expect(transactionHash).to.startsWith('0x')
+      expect(confCount).is.a('number')


Isn't there a possible race condition here since it is not guaranteed the confirmationCallback will get called before this line is executed ?

One possible way to test callbacks I think is to put the test code within the callback itself. See for ex: https://gist.github.com/eswak/c54989f05c6f2ce4be00
Maybe something along the lines of

confirmationCallback: (confirmationCount, transactionReceipt) => { expect(confirmationCount).is.a('number') expect(trReceipt).is.a('object') if (confirmationCount == NUMBER_CONFIRMATIONS_TO_REPORT) { // this test is done, go to the next one done() }

Ahh yes thanks @franckc . The approach with a done() callback is much better.

franckc · 2018-09-27T04:35:42Z

src/services/contract-service.js

@@ -13,6 +13,7 @@ import bs58 from 'bs58'
 import Web3 from 'web3'

 const emptyAddress = '0x0000000000000000000000000000000000000000'
+const NUMBER_CONFIRMATIONS_TO_REPORT = 20


Is 20 a bit too high ? I thought typically 6 to 10 confirmations is good ? Maybe we pick 10 ?

Yeah I also thought that initially and had it set to 5. Then I observed that DApp marks transaction as confirmed after 12 confirmations ( https://github.com/OriginProtocol/origin-dapp/blob/master/src/components/dropdowns/transactions.js#L8 ). So I took that information and applied some margin just to be safe. So if we are to reduce this 20 it needs to be at least bigger than 12...

Ok. Sg. Let's keep 20 for now then. Thanks for the details :)

My reason for 1️⃣:two:: https://ethereum.stackexchange.com/a/3009/42177

Thanks for explanation @micahalcorn

franckc · 2018-09-27T04:40:24Z

src/services/contract-service.js

@@ -239,6 +277,7 @@ class ContractService {
        .send(opts)
        .on('receipt', resolve)
        .on('confirmation', confirmationCallback)


Won't we have same issue here mentioned in the deploy method that confirmationCallback does not get triggered ? Are we relying on this callback anywhere in our DApp or origin-js codebase ? If yes let's fix it (ok to do in separate PR), if no could we at least add a comment to warn caller that this is currently not working ?

Interestingly when doing an Ethereum transaction confirmation callback is working, but when deploying a contract it is not. That is why the workaround is implemented only in the deploy function a few lines above.

Got it. Thanks for being thorough.

sparrowDom · 2018-09-27T08:48:00Z

@franckc responded to comments and improved the test as you have proposed.

And please don't hold back on the Pull Request comments. Each comment results either in clearing up confusion, improving the code base and/or improving coding skills. For that reason I prefer as many comments as possible.

…/origin-js into sparrowDom/tranactionHash

franckc

Nice work sir ! LGTM

sparrowDom added 2 commits September 25, 2018 16:13

add transaction hash event to contract call

3fd5f58

simpify api

95e164b

sparrowDom added the p0 Blocking priority label Sep 25, 2018

sparrowDom self-assigned this Sep 25, 2018

sparrowDom requested a review from franckc September 25, 2018 15:14

sparrowDom mentioned this pull request Sep 25, 2018

Show modal confirmation on transactionHash event instead of transaction receipt OriginProtocol/origin#577

Merged

2 tasks

franckc requested a review from tyleryasaka September 25, 2018 16:08

tyleryasaka approved these changes Sep 25, 2018

View reviewed changes

sparrowDom mentioned this pull request Sep 26, 2018

Create options object with callbacks #555

Open

sparrowDom added 4 commits September 26, 2018 21:08

add transactionHash and confirmationCallback to deploy function as well

f3610b2

handle null calllbacks

6e1cdc4

add possible probleme explanation

0bfcb1c

remove unneeded options

5f14c3a

franckc reviewed Sep 26, 2018

View reviewed changes

sparrowDom added 2 commits September 27, 2018 00:20

adding comments and abstracting constant

2cfc55c

add test for transactionHash and onComplete callbacks

697ac79

Merge branch 'master' into sparrowDom/tranactionHash

9340710

franckc reviewed Sep 27, 2018

View reviewed changes

handle possible race condition in test

fa7b66f

sparrowDom closed this Sep 27, 2018

sparrowDom reopened this Sep 27, 2018

franckc mentioned this pull request Sep 27, 2018

DApp modal does not go away after saving profile OriginProtocol/origin#571

Closed

Franck and others added 2 commits September 27, 2018 09:31

Merge branch 'master' into sparrowDom/tranactionHash

5f3e6f3

Merge branch 'sparrowDom/tranactionHash' of github.com:OriginProtocol…

59e7f26

…/origin-js into sparrowDom/tranactionHash

franckc approved these changes Sep 27, 2018

View reviewed changes

franckc merged commit 8e6f238 into master Sep 27, 2018

franckc deleted the sparrowDom/tranactionHash branch September 27, 2018 17:01

franckc mentioned this pull request Oct 2, 2018

After publishing profile, status remains "Not Published" OriginProtocol/origin#639

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sparrow dom/tranaction hash #546

Sparrow dom/tranaction hash #546

sparrowDom commented Sep 25, 2018 •

edited

Loading

franckc commented Sep 25, 2018

sparrowDom commented Sep 25, 2018

tyleryasaka left a comment

sparrowDom commented Sep 26, 2018

sparrowDom commented Sep 26, 2018

franckc left a comment

franckc Sep 26, 2018

franckc Sep 26, 2018

sparrowDom commented Sep 26, 2018

franckc left a comment

franckc Sep 27, 2018

sparrowDom Sep 27, 2018

franckc Sep 27, 2018

sparrowDom Sep 27, 2018

franckc Sep 27, 2018

micahalcorn Sep 27, 2018

sparrowDom Sep 27, 2018

franckc Sep 27, 2018 •

edited

Loading

sparrowDom Sep 27, 2018

franckc Sep 27, 2018

sparrowDom commented Sep 27, 2018

franckc left a comment

Sparrow dom/tranaction hash #546

Sparrow dom/tranaction hash #546

Conversation

sparrowDom commented Sep 25, 2018 • edited Loading

Checklist:

Description:

franckc commented Sep 25, 2018

sparrowDom commented Sep 25, 2018

tyleryasaka left a comment

Choose a reason for hiding this comment

sparrowDom commented Sep 26, 2018

sparrowDom commented Sep 26, 2018

franckc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sparrowDom commented Sep 26, 2018

franckc 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

franckc Sep 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sparrowDom commented Sep 27, 2018

franckc left a comment

Choose a reason for hiding this comment

sparrowDom commented Sep 25, 2018 •

edited

Loading

franckc Sep 27, 2018 •

edited

Loading