Origin: Return custom objects instead of raw web3 transactions from API

[wanderingstan]

EDIT: After discussion, we changed desired approach and title was updated. Old title was "Move waitTransactionFinished to origin object"

It's a common pattern to create and then wait for transaction. For example, this example from our docs:

const transaction = await origin.listings.close(
      listing.address
    )
await origin.contractService.waitTransactionFinished(transaction.tx)

However, we should strive to hide contractService from developers, so I'd suggest moving waitTransactionFinished() directly to the origin object, or making a method on origin that relays the call to contractService.

EDIT: We should not return a raw web3 transaction, but should return an abstracted transaction object with a convenience function isFinished() for waiting for it to finish.

The above line would then read

const transaction = await origin.listings.close(
      listing.address
    )
await transaction.isFinished()

@DanielVF what do you think?

[tyleryasaka]

@wanderingstan Couple thoughts.

1. If we're wanting to hide contractService from developers, then we shouldn't export it, right?

2. I like this approach; however, I think if we're dividing the origin package into smaller modules, then maybe we want to place all of our methods into modules? So instead of putting this method directly on the origin object, maybe we could put it in a separate package. (Something like util maybe?) Not entirely sure about this - need to look at the code a bit more first. But just trying to think about keeping everything clean and organized.

[wanderingstan]

I may be opening a bigger can of worms than intended here. :)

In general, our promise is "origin.js users don't need to know anything about blockchain or IPFS, they just use our library". So I'd consider exposing contractService for such a common pattern to be a slight breaking of that promise.

On second glance, perhaps its already a misstep that our methods return a web3 transaction. Can we instead return an abstracted "origin transaction", with a convenience method like isFinished()? Then the above code might look like this:

const transaction = await origin.listings.close(
      listing.address
    )
await transaction.isFinished()

[DanielVF]

@wanderingstan I agree with you!

Returning the web3 transaction should be a temporary thing - we should definitely return our own thing instead.

[wanderingstan]

@DanielVF cool. I'll rename this issue to reflect this design choice, and we'll put it in the backlog for a future sprint.

[DanielVF]

I'm coding this as we speak. 🛩

[wanderingstan]

_NOTE: There was discussion on discord around possibly naming the function whenMined_

I don't like whenMined, as it requires our developers know something about blockchain, again breaking our promise that origin.js users don't need to know blockchain, only markerts.

All they care about is whether the thing is finished / done / complete. "Mined" doesn't mean much outside of the crypto world.

EDIT: isConfirmed was also suggested, which is better, though I still prefer something like isComplete.

As for number of blocks, I'd suggest we have a reasonable default set in the configuration, but allow advanced users to modify it. We could also expose the number of blocks (or the raw web3 event) for advanced users as well.

[tyleryasaka]

While I'm not super opinionated on this (hence my flip-flopping), I do tend to agree with @wanderingstan here. I think he makes a good case for keeping the name more generic. In terms of interfaces, I think a good principle is to hide as many details as possible. And origin.js of course is an interface. Do users of the exported class actually need to know that a block has been mined in order to use it? No. They just need to know that the thing has happened. Of course some people might be curious what's actually going on under the hood - and I believe proper documentation would be the place to explain that.

[wanderingstan]

I believe we're done here...right @DanielVF ? Can this be closed?

[DanielVF]

No, not ready to be closed.

We're still returning what comes back from web3, plus some one of our own annotations. We've not decided yet exactly what we'll be returning.

[micahalcorn]

Started in https://github.com/OriginProtocol/origin-js/pull/285/files#diff-c98ea273c33570c10992bb27cc43b7edR203 ☑️

[micahalcorn]

Closing this because GraphQL 🤷‍♂️