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.

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

events refactor #174

Merged
merged 26 commits into from
Apr 22, 2015
Merged

events refactor #174

merged 26 commits into from
Apr 22, 2015

Conversation

debris
Copy link
Contributor

@debris debris commented Apr 20, 2015

changes

  • event.js && contract.js refactored
  • new file function.js
  • indexed params are now null if not specified

@coveralls
Copy link

Coverage Status

Coverage increased (+0.86%) to 94.95% when pulling eeb0bc0 on events_refactor into cdf02ec on develop.

var typeName = json.inputs.map(function(i){return i.type; }).join();
return json.name + '(' + typeName + ')';
};

Copy link
Contributor

Choose a reason for hiding this comment

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

I would probably call this abi instead of json for clarity.

@frozeman
Copy link
Contributor

Please see my answers on you comments.

Additionally we need to fix the events output to match the docs in event.js line 120 from:

    var result = {
        event: this.displayName(),
        number: data.number,
        hash: data.hash,
        args: {}
    };

to:

    var result = {
        event: this.displayName(),
        args: {},
        logIndex: data.logIndex,
        transactionIndex: data.transactionIndex,
        transactionHash: data.transactionHash,
        address: data.address,
        blockHash: data.blockHash,
        blockNumber: data.blockNumber
    };

And probably run data first through the log output formatter.

Conflicts:
	dist/web3-light.js.map
	dist/web3-light.min.js
	dist/web3.js.map
	dist/web3.min.js
@debris
Copy link
Contributor Author

debris commented Apr 21, 2015

fields are fixed in latest pr

@debris
Copy link
Contributor Author

debris commented Apr 21, 2015

now I will do

myContract.myFunction.sendTransaction({value: 20});
// and
myContract.myFunction.call();

@debris
Copy link
Contributor Author

debris commented Apr 21, 2015

once this pr is merged we will upgrade version to 0.3.0

@coveralls
Copy link

Coverage Status

Coverage increased (+0.25%) to 94.34% when pulling e910736 on events_refactor into 42e759e on develop.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.63%) to 94.73% when pulling 1ac1ef9 on events_refactor into f37057e on develop.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.26%) to 95.36% when pulling 9f7d6a9 on events_refactor into f37057e on develop.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.27%) to 95.37% when pulling 3d3db61 on events_refactor into f37057e on develop.

@frozeman
Copy link
Contributor

The contract tests are still the same showing e.g.:

contract.call({from: address, gas: 50000}).balance(address);

This shouldn't work anymore, or do we have backwards compatibility?

Can we add contract tests which also use the ..myMethod.sendTransaction() and ..myMethod.call() syntax?

@frozeman
Copy link
Contributor

events.js:120 is still the same, and has not the necessary properties.

EDIT: i take it back, it didn't pull correctly for me. But i think its better to use the proper log output formatter before, than doing it manually in the event.js again.

As it is we would need to update two places when we change the log output

@debris
Copy link
Contributor Author

debris commented Apr 22, 2015

Can we add contract tests which also use the ..myMethod.sendTransaction() and ..myMethod.call() syntax?

done

debris added a commit that referenced this pull request Apr 22, 2015
@debris debris merged commit e21ee7a into develop Apr 22, 2015
@coveralls
Copy link

Coverage Status

Coverage increased (+1.27%) to 95.37% when pulling 960e9c9 on events_refactor into f37057e on develop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants