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

Sparrow dom/tranaction hash #546

Merged
merged 12 commits into from
Sep 27, 2018
4 changes: 2 additions & 2 deletions src/contractInterface/users/resolver.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ class UsersResolver {
this.currentAdapter = this.adapters[this.currentVersion]
}

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

async get(address) {
Expand Down
11 changes: 6 additions & 5 deletions src/contractInterface/users/v00_adapter.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,13 @@ class V00_UsersAdapter {
this.contractName = 'V00_UserRegistry'
}

async set({ profile, attestations = [] }) {
async set({ profile, attestations = [], options = {}}) {
if (profile) {
const selfAttestation = await this.profileAttestation(profile)
attestations.push(selfAttestation)
}
const newAttestations = await this.newAttestations(attestations)
return await this.addAttestations(newAttestations)
return await this.addAttestations(newAttestations, options)
}

async get(address) {
Expand Down Expand Up @@ -91,7 +91,7 @@ class V00_UsersAdapter {
})
}

async addAttestations(attestations) {
async addAttestations(attestations, options) {
const account = await this.contractService.currentAccount()
const userRegistry = await this.contractService.deployed(
this.contractService.contracts[this.contractName]
Expand Down Expand Up @@ -123,7 +123,7 @@ class V00_UsersAdapter {
'ClaimHolderRegistered',
'addClaims',
[topics, issuers, sigs, data, dataOffsets],
{ from: account, gas: 400000, contractAddress: identityAddress }
{ ...options, from: account, gas: 400000, contractAddress: identityAddress }
)
} else {
// create identity with presigned claims
Expand All @@ -138,7 +138,8 @@ class V00_UsersAdapter {
data,
dataOffsets
],
{ from: account, gas }
{ from: account, gas },
options
)
}
} else if (!identityAddress) {
Expand Down
8 changes: 6 additions & 2 deletions src/resources/users.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,12 @@ class Users {
this.resolver = new UsersResolver({ contractService, ipfsService })
}

async set({ profile, attestations = [] }) {
return this.resolver.set({ profile, attestations })
/* possible options values:
* - confirmationCallback(confirmationCount, transactionReceipt) -> called repeatedly after a transaction is mined
* - transactionHashCallback(hash) -> called immediately when the transaction hash is received
*/
async set({ profile, attestations = [], options = {}}) {
Copy link

Choose a reason for hiding this comment

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

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

return this.resolver.set({ profile, attestations, options })
}

async get(address) {
Expand Down
43 changes: 41 additions & 2 deletions src/services/contract-service.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import bs58 from 'bs58'
import Web3 from 'web3'

const emptyAddress = '0x0000000000000000000000000000000000000000'
const NUMBER_CONFIRMATIONS_TO_REPORT = 20
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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...

Copy link

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for explanation @micahalcorn

const SUPPORTED_ERC20 = [
{ symbol: 'OGN', decimals: 18, contractName: 'OriginToken' }
]
Expand Down Expand Up @@ -192,7 +193,7 @@ class ContractService {
return withLibraryAddresses
}

async deploy(contract, args, options) {
async deploy(contract, args, options, { confirmationCallback, transactionHashCallback } = {} ) {
const bytecode = await this.getBytecode(contract)
const deployed = await this.deployed(contract)
const txReceipt = await new Promise((resolve, reject) => {
Expand All @@ -205,16 +206,53 @@ class ContractService {
.on('receipt', receipt => {
resolve(receipt)
})
//.on('confirmation', confirmationCallback)
//.on('transactionHash', transactionHashCallback)
// Workaround for "confirmationCallback" not being triggered with web3 version:1.0.0-beta.34
.on('transactionHash', (hash) => {
if (transactionHashCallback)
transactionHashCallback(hash)
if (confirmationCallback)
this.checkForDeploymentCompletion(hash, confirmationCallback)
})
.on('error', err => reject(err))
})
return txReceipt
}

/* confirmation callback does not get triggered in current version of web3 version:1.0.0-beta.34
* so this function perpetually (until 20 confirmations) checks for presence of deployed contract.
*
* This could also be a problem in Ethereum node: https://github.com/ethereum/web3.js/issues/1255
*/
async checkForDeploymentCompletion(hash, confirmationCallback) {
const transactionInfo = await this.web3.eth.getTransaction(hash)

// transaction not mined
if (transactionInfo.blockNumber === null){
setTimeout(() => {
this.checkForDeploymentCompletion(hash, confirmationCallback)
}, 1500)
} else {
const currentBlockNumber = await this.web3.eth.getBlockNumber()
const confirmations = currentBlockNumber - transactionInfo.blockNumber
confirmationCallback(confirmations, {
transactionHash: transactionInfo.hash
})
// do checks until NUMBER_CONFIRMATIONS_TO_REPORT block confirmations
if (confirmations < NUMBER_CONFIRMATIONS_TO_REPORT) {
setTimeout(() => {
this.checkForDeploymentCompletion(hash, confirmationCallback)
}, 1500)
}
}
}

async call(
contractName,
functionName,
args = [],
{ contractAddress, from, gas, value, confirmationCallback, additionalGas = 0 } = {}
{ contractAddress, from, gas, value, confirmationCallback, transactionHashCallback, additionalGas = 0 } = {}
) {
const contractDefinition = this.contracts[contractName]
if (typeof contractDefinition === 'undefined') {
Expand All @@ -239,6 +277,7 @@ class ContractService {
.send(opts)
.on('receipt', resolve)
.on('confirmation', confirmationCallback)
Copy link

@franckc franckc Sep 27, 2018

Choose a reason for hiding this comment

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

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 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

Got it. Thanks for being thorough.

.on('transactionHash', transactionHashCallback)
.on('error', reject)
})
const block = await this.web3.eth.getBlock(transactionReceipt.blockNumber)
Expand Down
31 changes: 31 additions & 0 deletions test/resource_users.test.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import chai from 'chai'
import chaiString from 'chai-string'
import chaiAsPromised from 'chai-as-promised'
import Web3 from 'web3'

Expand All @@ -9,6 +10,7 @@ import ContractService from '../src/services/contract-service'
import IpfsService from '../src/services/ipfs-service'

chai.use(chaiAsPromised)
chai.use(chaiString)
const expect = chai.expect

const issuerPrivatekey =
Expand Down Expand Up @@ -220,6 +222,35 @@ describe('User Resource', function() {
const badProfile = { profile: { bad: 'profile' } }
return expect(users.set(badProfile)).to.eventually.be.rejectedWith(Error)
})

it('should be able to receive transactionHash and onComplete callbacks', (done) => {
let transactionHash
let doneCalled = false

users.set({
profile: {
firstName: 'Wonder',
lastName: 'Woman',
},
options: {
transactionHashCallback: (hash) => {
transactionHash = hash
},
confirmationCallback: (confirmationCount, transactionReceipt) => {
expect(confirmationCount).is.a('number')
expect(transactionReceipt).is.a('object')
// transactionHashCallback should always execute before confirmationCallback
expect(transactionHash).to.startsWith('0x')

// prevent done being called multiple times
if (!doneCalled){
doneCalled = true
done()
}
}
}
})
})
})

describe('get', () => {
Expand Down