-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
web3.js contract object re-design #68
Comments
The gist of changes (please correct me if I'm wrong):
The I welcome the I don't quite understand the rational behind the |
Additional ideas: We can think of using a The only drawback in this proposal right now is that you have no |
Nice proposal! Some ideas: Events given as string: I think it's fine, but should also allow for parameter types to disambiguate and throw if the event does not exist.
Inside getABI we should use For event filters: Can we please make it clear in the syntax whether we AND or OR the arguments? Do we still allow I think it is a good idea to separate the actual function arguments and the metadata like gas and sender. |
Thanks for you comment. I changed it to The old For the last comment, i added Is that fine, or what do you mean.? |
Looks nice! 😄 So in this usage eventEmitter.on('mined', function(address) {
new web3.eth.contract(abi, address);
// or
myContract.address = address;
}); you only get the address once the transaction creating the contract has been mined. Sometimes it might be desirable to create a contract and immediately send a transaction to it. Since the address only depends on the nonce the address is known right after the transaction is sent off. It might be useful to have the API return the address after the transaction is sent. Would that create other issues? |
It is most likely the right address, if you predetermine it from the nonce, but there are cases where it could fail (e.g. a different tx got in before you etc, but then it might fail right away). I will think about adding it. |
The API doesn't seem to handle microforking (i.e. chain reversions for these events) - any plans for that? Or suggested workarounds? |
They will, the callback is fired with the log object, which will have the |
I very much approve of the handling of structs. This will make SO many things easier when it comes to solidity. |
I'd like to introduce ether-pudding (a.k.a., "Pudding") for those that aren't familiar. Pudding is a similar abstraction layer for Web3, but it goes beyond what Web3's abstraction layer does, and even this ERC. My end goal for Pudding was always for it to not exist, and so I hope that its features can eventually get consumed. Regarding the ERC, I'm very excited about
Given all the features I mentioned together, interacting with contracts becomes much easier, and in total there's less to think about. Here's a Pudding example: // Read in the contract from a file that contains the abi, binary, and other metadata.
// .load() is an idiosyncrasy; we need to ensure the same Pudding instance is used throughout
var MyContract = require("./compiled-contracts/MyContract.sol.js");
MyContract.load(Pudding);
// Set the defaults class-wide, and not just instance-wide.
MyContract.defaults({
from: "0x1234..."
});
// Create a new version of the contract, without having to worry about the details
var myContract;
MyContract.new(function(instance) {
// Instance is now on the blockchain, and can be interacted with
myContract = instance;
// Call someFunction() on the instance.
return myContract.someFunction();
}).then(function(tx_hash) {
// Now read some data, since that transaction has been mined.
return myContract.readSomeData.call();
}).then(function(some_data) {
// Do something with the data we just read from the network.
...
}).catch(function(e) {
// Handle errors throughout the whole promise chain
}); I think we have a chance to make interacting with the blockchain less tedious for the developer. Pudding is a step forward, but I'd very much hope Pudding doesn't have to exist. This ERC still requires the developer to think about a lot of moving parts, like mining, or dealing with the hash, etc. I think in general we should focus on reducing the amount of things the developer has to think about, and work on making the resulting code clearer. Aside: You can think about mining, transaction hashes, _not_ waiting for a transaction to be mined, etc. with Pudding, if you want to. But that's not Pudding's -- and I think our -- main goal. |
@tcoulter is spot on. Promised are an awesome addition and should be a standard for web3. |
👍 for @tcoulter's points! Pudding is very nice, I use it for all my web3 projects. Good that we have callback on mined transaction, Promises would be very nice in web3! |
@tcoulter i agree with all of your points, though with slight differences:
Thanks again for all the great feedback. If anybody want to help me integrating these new features, please shot me a message, as i'm only one person with two hands and 3 projects ;) |
Follow the progress: https://github.com/ethereum/web3.js/blob/newContract/lib/web3/contract.js I would appreciate any help. |
@frozeman The way I understand promises in pudding, it does just that in waiting for the transaction to be mined which then sends a callback, and then the promise clears and moves onto the next step. Furthermore, why should serializing contracts be in a separate tool, just out of curiosity? |
Great @frozeman, glad this helps! Regarding this point:
Pudding accomplishes this by having two different functions, one that waits to be mined and one that doesn't. In my experience, I use the function that waits to be mined 99% of the time. Here's an example:
The default behavior for Pudding is to wait until the transaction has been mined, however you can circumvent this by calling Regarding @VoR0220's question:
I'm assuming that's because it adds a lot of dependencies to web3, and were it to perform all the serialization functionality web3 would have to write to the filesystem, which only works in some environements (i.e., Node). I'm fine with this, actually. Pudding would then become a code management tool more than an abstraction layer, which is a fine separation, I think. Cheers! |
I agree that chaining for function calls etc is nice, and i'm all for promises. In fact i wrote a whole API layer on promise basis a few years back. But in this case i don't think promises, or two separate functions is the right way. The advantage of the event emitter is that it allows us to later add even more events, which can then even be subscriptions under the hood. It allows to use web3.js low level, as well as convenient while having a clear API. Promises can only do one action after a certain time, and are therefore only really useful when we do contract function and method calls. |
I agree with all your points, but in my experience "doing contract function and method calls" is 99% of my interaction with the Ethereum blockchain. We can still emit events for more advanced things, but I think we should focus the main use case on the features people will use most often. Regarding promises specifically, I don't care if we use promises or some other chaining method, but I'd really like to avoid code like this, which is how most of my code would become:
This seems less clear. We can have events and chaining (chaining without promises). It's possible, and I'd be happy to try and code up an example. |
I'd be very happy with these changes, but I'm just concerned that they're changes. I'd have a large amount of code that would need rewritten if this API replaced the current API, and I'm sure other projects would have similar amounts of code to rewrite. Can we work some under-the-hood JS magic and have web3.eth.contract return (deprecated) old-style objects if simply called and new-style contracts if new is used? (I have no idea if this is even possible.) Alternately, if that is impossible/would awaken foul horrors from the depths of time, could we get away with specifying the version somewhere? I suppose, however, that inevitably that web3 has to make breaking changes, (Like, say, when web3.sha3 works as one expects it would.) For my own major project, I get web3 from browserify, so I could probably just specify the version using npm. The real question is: how will mist provide web3 in the future, and how much guarantee will developers have that web3 will remain stable? |
@tcoulter i understand you callback hell fear, but only returning promises, which are resolved when its mined limits you for when you need the tx has directly.. How would you allow chaining while being flexible?? @Smithgift we will need to make breaking changes, and users can choose their own web3.js version. Concerning Mist. Ideally the dapp developers always uses its own web3.js version. The mist is only there for the simple dapps |
@tcoulter do you think about some crazy promise style event like? contract.transact().myMethod().on('mined').then(function(){ }) |
@frozeman I was more thinking something like this, which returns a promise-like object that's also an event emitter: var transaction = contract.transact().myMethod();
transaction.on("transactionHash", function() {
// This function will be called when the transaction is returned
// (i.e., once transaction is submitted)
});
transaction.on("mined", function() {
// This function will be called when the transaction is mined
});
transaction.then(function(tx_hash) {
// This is equivalent to transaction.on("mined"), and fires at the same time.
// Here's where the chaining happens: returning a new transaction object for
// the next transaction, like you can do with promises.
return contract.transact().otherMethod();
}).then(function(tx_hash) {
// Again, this won't be fired until the transaction is mined.
// In addition, we should support calls in chainable format. This is arguably
// more important as I make more calls than transactions, one after the other:
return contract.call().yetAnotherMethod();
}).then(function(return_value) {
// Do something with the call's return value.
}).catch(function(e) {
// It's important that we support catching errors across the whole chain, like promises.
}); Implementation-wise, I think there are two avenues to achieving this:
I think both are doable. If we choose the first, we should be sure to use the same Promise interface so we can easily integrate with other promise libraries and get all the benefit of Promises without having to completely rewrite the whole library (for instance, I use |
PS: I'm on the edge of my seat whether you end up liking this. So excited if this gets in. Also happy to help with implementation. |
👍 @tcoulter 's proposal. I really, really think we should enable some kind of promise into the transaction sending. It makes coding and testing for these things soooooo much easier. |
Sounds like a good idea. I already started here: with the eventEmitter, we could simply add promise support then i guess. I used Q from kritokwal a while back |
@tcoulter this was an awesome idea! var eventifiedPromise = function() {
var resolve, reject,
emitter = new EventEmitter(),
promise = new Promise(function() {
resolve = arguments[0];
reject = arguments[1];
});
// add eventEmitter to the promise
promise.emit = emitter.emit;
promise.on = emitter.on;
promise.once = emitter.once;
promise.listeners = emitter.listeners;
promise.addListener = emitter.addListener;
promise.removeListener = emitter.removeListener;
promise.removeAllListeners = emitter.removeAllListeners;
return {
resolve: resolve,
reject: reject,
promise: promise
};
}; I implemented and tested it with This now allows deploy (and later other contract methods) to allow: callbacks, promises and events!! So everybody can choose which one to use. Example: myContract.deploy({from: '0x12345...', data: '0x234567...', arguments: [123]}, function(e, res){ console.log('Callback results:', e,res)});
event.on('error', function(error){ console.log('Event error:', error)});
event.on('transactionHash', function(hash){ console.log('Event transactionHash:', hash)});
event.on('mined', function(address){ console.log('Event mined:', address)})
event.then(function(address){ console.log('Pomise mined:', address) }).catch(function(e){ console.log('Promise error: ', e); });
> Promise {[[PromiseStatus]]: "pending", [[PromiseValue]]: undefined}
> Callback results: null 0xcea73979a424ebd2c1e4ecdd0844c5cb4644e92cf04f7e5f1cc5d87e69ee5168
> event transactionHash: 0xcea73979a424ebd2c1e4ecdd0844c5cb4644e92cf04f7e5f1cc5d87e69ee5168
> Event mined: 0xa1fb3680c055ba95f24bacb59ed1b32cbc699326
> Promise mined: 0xa1fb3680c055ba95f24bacb59ed1b32cbc699326 BTW i clear all event listeners automatically after an error or after the mined event, as no other events will ever be fired. |
Made contract.encodeABI({method: 'constructor', arguments: ['0x23456789'], data: '0x12345678912345678'})
> "0x123456789123456780000000000000000000000000000000000000000000000000000000023456789"
contract.encodeABI({method: 'seller', arguments: [2]})
> "0x37ab20b00000000000000000000000000000000000000000000000000000000000000002" |
This all sounds great. What kind of time frame are we looking at for this release? |
@rfikki the contract object might be done soon, but i have some more changes planned for web3.js and we move completely to pub/sub (PR already ready) Parts of this also depend on how fast we get changes into geth and other clients. @tcoulter i used native promises, as you can see here: web3/web3.js@9878c86 Though i'm not sure if i should load bluebird as backup, e.g. for IE. Node.js works fine with promises, at least the version i have running. |
In, re: clearing all events, I would be a little cautious before clearing the "mined" events, just in case there's a microfork and the transaction is reverted. Clearing all on error makes sense. |
I see your point. But even if there is a microfork, you wouldn't get the mined event fired again, as its a event internally in web3.js, which once fired is "done". Though i can imagine moving the mined event at some point to the pub/sub as a subscription inside the node, then this might make sense. I could also add checking and confirmation events later on. The good thing about the current structure is that we can improve the under-the-hood behaviour, as well as add new event types |
Great work guys, I like pudding's style a lot and I'm glad chained promises will become standard. Also +1 to @frozeman's suggestion:
This will make things like progress bars really easy to work with. Would there also be a way to access this data without an event emitter, for longer confirmation times; perhaps something like an |
@iurimatias this is something to absolutely keep an eye on for Embark. |
@hitchcott can you elaborate on your last idea a bit? |
Let's say there's a contract that needs 100 confirmation blocks and for whatever reason the event emitter is stopped (user closes window, navigates to different route). If the user navigates back to the contract view, we want to immediately show the number of confirmation blocks without waiting for a |
Suggestion for the future (I think this isn't possible with the current EVM): a "out-of-gas" event. It's not technically an error, but it's worth telling the developer. (Really, you're best off listening for actual contract events and acting on them, rather than assuming a transaction did what it was supposed to. But going out-of-gas is a significant event.) |
@hitchcott this would require a lot of calls under the hood, which i would like to keep out of web3.js for now. @Smithgift good idea to add the out of gas, event |
@frozeman @hitchcott Please make web3.js extensible so that users can add their own logic regarding transactions e.g. @hitchcott 's example. I have low confidence and high confidence code (i.e. I don't care if an Oracle update doesn't get mined, but I care a lot if ether didn't actually get sent) and we need to be able to differentiate the two. I'm fine writing this myself, but only if web3.js makes it easy to do without a pile of logic in my application code. For example, a wrapper function where I consume a Another area is sending maxGas is fraught with peril as currently an exception will consume USD $1 of gas. Yikes. I'd like to be able to monitor how close to gasSent an API call is getting but I definitely don't want that code in my application logic. If I can extend web3's notion of a transaction with my own logging magic I'm fine writing it myself... but at least make that possible. Right now the concept of which solidity API got called and how much gas was used are in completely separate areas of the code, which means I'm littering my application logic with that type of code. You could for example make it easy to attach the ABI signature to the transaction receipt, then it's a simple from the application side, and a separate concern for logging the different between gasSent and gasUsed. A third example is most applications won't care all that much about the transaction hash the application logic. The application really should get an exception if a transaction isn't mined in a reasonable amount of time, and other than that application logic doesn't care much about the transaction results. In fact the only "returned" part of a completed transaction that application logic will care about is the txlogs, which currently aren't parsed. Make it easy for the user to extend the transaction code in web3.js to add parsing of the txlogs without that logic littering up the application code. @tcoulter posted a great example on stackexchange of how to do this. See how hard it would be for an end-user to add that code to web3.js or a wrapper on web3.js as a test case. Again, I'm not asking you to code these features, I'm asking you to make it easy for the user to code these features without having to litter them into their application logic. |
Another extensibility comment: rxjs allows you to define which promise library to use. bluebird IMHO is more efficient and easier to debug than native. So allow the user to define what promise library to use. from example straight out of my code:
|
@barkthins i agree with the flexibility argument. Could you provide some example code of what it is you would like to have, its hard for me to understand your problem if you write a bunch of texts of what you like to have. Also could you link @tcoulter stack overflow issue? Also keep in mind, web3.js is a low level API library not an all in wonder solution for every dapp, so adding more logic will grow it internally and i try to keep the calls we do internally to the node at a minimum. Additionally could you explain me you promise logic? I use bluebird as a fallback currently, but how would you configure the promise library from the outside? you want web3.js to expose the Rx object? I'm want to keep web3.js lean and not add any additional new cool library.. |
@frozeman let's start a conversation on gist, I have example code there. Let's try and modify it to meet the requirements I listed. |
No, that was an example of how to specify a promise library for a module. RXJS definitely should not be in web3.js. I hook RXJS off of the event callbacks and it works great, no need to put that into web3.js since it's easy for users to add as needed. |
Is this still happining? I was planning on updating my python-based tooling to conform to this API but I don't want to put in the effort unless this is going to be real. |
Oh, how I would love to have proper promises. Making transactions and events chainable would improve my code a lot. Would that also allow us to add something like |
Having promises as a default tool for Any update on this? |
@Schaeff There's this https://github.com/SafeMarket/web3-q but it wraps the default web3 functions rather than overwriting them for upgrade reasons. |
@frozeman Can be closed? Or do you prefer to wait until web3/web3.js#558 (web3.js 1.0) is merged? |
Closing this as the web3.js 1.0 branch has become the default. |
Abstract
This proposal is a move to make it a more future proof javascript representation of a ethereum smart contract.
Motivation
The contract object in web3.js in its current form has confused in many ways and has some draw backs, like not being able to pass a struct (object) as a functions arguments.
Usage
Summary
The contract object would then look as follow:
Community Request
Please give comments and reasons, why we should go in one way or another. Please lets keep this ERC short, We don't want to spend multiple weeks discussing this ;)
The text was updated successfully, but these errors were encountered: