tasitlabs / tasit-apps

Native mobile Ethereum dapps for mainstream users
https://tasit.io/
MIT License
36 stars 9 forks source link

Handle ABIs in the demo app and/or SDK the way we'll want developers to #82

Open marcelomorgado opened 5 years ago

marcelomorgado commented 5 years ago

Refs: https://github.com/tasitlabs/tasit/pull/79#discussion_r254316925 Extracted message:

I forget, did you have thoughts on getting the ABIs from over in the SDK in the future?

Yes, maybe we can expose ABIs when Decentraland package will be available;

I suppose maybe that's not the best demo of how an external user would use it, though.

Using the Contract class with their own ABI isn't the user will do something like that? Maybe expose some ABIs (20,721,etc) as constants?

Mentioning this tool in case it's useful to think about, since it's kind of on this topic https://github.com/JoinColony/trufflepig

Maybe we have to think on both scenarios: 1) When user under develop process using the SDK (local blockchain / abi from truffle/trufflepig) 2) When SDK is using to interact with a pre-existing mainnet (without ganache-cli / not deployed by user)

Brainstorm Note: When I was writing item 2, I've thought that we can even suggest to our users development stages when interact with 3rd-party contracts. As we are doing with Decentraland:

ABI Note: What do you think about ethers.js human readable ABI? Should we allows users instante contracts using that?

marcelomorgado commented 5 years ago

Sorry, again that adds to the milestone automatically. Is that belongs to v0.1.0?

pcowgill commented 5 years ago

@marcelomorgado I think we can have this as a 0.2.0 task.

Also it seems that the new issues haven't been getting assigned to the Main project. Can you add them to that project in the future? Thanks!

pcowgill commented 5 years ago

Refs: #79 (comment) Extracted message:

I forget, did you have thoughts on getting the ABIs from over in the SDK in the future?

Yes, maybe we can expose ABIs when Decentraland package will be available;

Ah okay, you're thinking we'll publish a tasit-decentraland contracts package? Or did you mean getting it from tasit-contracts?

I suppose maybe that's not the best demo of how an external user would use it, though.

Using the Contract class with their own ABI isn't the user will do something like that? Maybe expose some ABIs (20,721,etc) as constants?

I think if the user can get away with just using the default features of ERC721, they won't want to both adding an ABI.

In general we probably won't have the contracts the user wants included in the TasitSDK repo yet, so often users will want to supply their own ABI.

I can't decide what we should do in this special case where the ABI we want is there. I think maybe in the future in the demo app it should supply the ABI manually, but in the Tasit Decentraland app we can import it from the Tasit SDK?

Maybe if they can bolt on just the human readable methods that are supported on top of ERC20, ERC721, that would be a good DevEx.

pcowgill commented 5 years ago

Mentioning this tool in case it's useful to think about, since it's kind of on this topic https://github.com/JoinColony/trufflepig

Maybe we have to think on both scenarios:

1. When user under develop process using the SDK (local blockchain / abi from `truffle`/`trufflepig`)

2. When SDK is using to interact with a pre-existing mainnet (without ganache-cli / not deployed by user)

Yeah breaking it down by scenario is helpful, and those seem like two good ones (although I'm not sure how popular TrufflePig is yet).

Brainstorm Note: When I was writing item 2, I've thought that we can even suggest to our users development stages when interact with 3rd-party contracts. As we are doing with Decentraland:

Great idea!!!

* Local deployment from same code and test as contract owner

I still think they shouldn't run the tests as the contract owner. They should switch to a different user. Other than that I agree.

* Local network fork from testnet (We will do that and test suite should be changed because we'll no longer been owner of contracts);

This could be good for integration tests.

* Testnet network (Should be a smooth move from last step)

This would be what we use for testing internally and with alpha testers with TestFlight

* Local network fork from mainnet;

* Mainnet network.
  Assuming that the 3rd-party keeps both testnet/mainnet as Decentraland does.

I like the idea of documenting this! And these steps make sense to me. Where do you think these docs belong?

ABI Note: What do you think about ethers.js human readable ABI? Should we allows users instante contracts using that?

I like it! I think we should. (Maybe even making our own even simpler abstraction on top of it if appropriate.)

marcelomorgado commented 5 years ago

Refs: #79 (comment) Extracted message:

I forget, did you have thoughts on getting the ABIs from over in the SDK in the future?

Yes, maybe we can expose ABIs when Decentraland package will be available;

Ah okay, you're thinking we'll publish a tasit-decentraland contracts package? Or did you mean getting it from tasit-contracts?

Hmm, I think that "package" wasn't a good word. What I've said is when we have Decentraland level of abstraction, for example: new DecentralandMarketplace() (or somethink like that), Make sense give as "utils constants" the addresses of the contracts envolved?

I suppose maybe that's not the best demo of how an external user would use it, though.

Using the Contract class with their own ABI isn't the user will do something like that? Maybe expose some ABIs (20,721,etc) as constants?

I think if the user can get away with just using the default features of ERC721, they won't want to both adding an ABI.

Sure. I'm wondering if there are others standard contracts that we'll not necessarly have an object like NFT, and maybe give these ABIs to user. But it isn't make sense since seems better to expose ABC = addr => new Contract(addr, abcABI) instead of abcABI only.

In general we probably won't have the contracts the user wants included in the TasitSDK repo yet, so often users will want to supply their own ABI.

I can't decide what we should do in this special case where the ABI we want is there.

Thinking on how Embark and Drizzle do that, both use ganache as dependency. Drizzle uses truffle on the project and point to build/contracts dir Embark do the same (But seems it doesn't use truffle). Summarizing: Both has all stack on the same project.

Our difference here, seems that live on the facts that we are using another device and tasit-contract (truffle) won't be a tasit app dependency. Said that, maybe the key to solve this is prepare the ground on start process (check if ganache is running, get contracts abi and addresses and so on).

I think maybe in the future in the demo app it should supply the ABI manually,

Maybe a setup tool/feature that gets ABI from a given contract address (from etherscan).

but in the Tasit Decentraland app we can import it from the Tasit SDK?

Looking what we are doing now from the point of view that we are creating an dapp using TasitSDK.Contract for Decentraland (that we'll support) but it could be another dapp that we'll not support. Maybe we should do the same as user will do (handle with ABI ourselves). On the other hand, since we are tracking these contract already, we can expose that ABIs, or even a Decentraland contracts objects (as a draft from top level of abstraction) but it'll only a subclass from TasitSDK.Contract for now.

Maybe if they can bolt on just the human readable methods that are supported on top of ERC20, ERC721, that would be a good DevEx.

:+1:

pcowgill commented 5 years ago

Hmm, I think that "package" wasn't a good word. What I've said is when we have Decentraland level of abstraction, for example: new DecentralandMarketplace() (or somethink like that), Make sense give as "utils constants" the addresses of the contracts envolved?

Got it. Makes sense. Yeah the addresses as well as the ABI too.

Sure. I'm wondering if there are others standard contracts that we'll not necessarly have an object like NFT, and maybe give these ABIs to user.

Yes, I think so. I agree that exposing the class with the ABI already inside makes more sense.

Thinking on how Embark and Drizzle do that, both use ganache as dependency. Drizzle uses truffle on the project and point to build/contracts dir Embark do the same (But seems it doesn't use truffle). Summarizing: Both has all stack on the same project.

Ah ok, good to know. Thanks for including this info for comparison. I'll continue to ponder what it means for us.

Our difference here, seems that live on the facts that we are using another device and tasit-contract (truffle) won't be a tasit app dependency. Said that, maybe the key to solve this is prepare the ground on start process (check if ganache is running, get contracts abi and addresses and so on).

tasit-contract could be a tasit app dependency for users that don't want to use the entire SDK but want to use it modularly instead.

And yeah, preparing things on process start sounds like a good approach to me.

Maybe a setup tool/feature that gets ABI from a given contract address (from etherscan).

Yeah, I like the direction you're picturing here. In the tasit repo demo dir? Or in the tasit repo as a a shared tool that all the apps there draw from (maybe the CLI, maybe not)? Or in a shared tool that the demo app uses by having tasit-sdk as a dependency?

Looking what we are doing now from the point of view that we are creating an dapp using TasitSDK.Contract for Decentraland (that we'll support) but it could be another dapp that we'll not support. Maybe we should do the same as user will do (handle with ABI ourselves). On the other hand, since we are tracking these contract already, we can expose that ABIs, or even a Decentraland contracts objects (as a draft from top level of abstraction) but it'll only a subclass from TasitSDK.Contract for now.

Yeah, I'm continuing to think using an ABI in the demo app but then getting the new DecentralandMarketplace() in the decentraland app that is where much of the code from the demo app will eventually move is the way to go.

šŸ‘

šŸ‘

marcelomorgado commented 5 years ago

A)

Hmm, I think that "package" wasn't a good word. What I've said is when we have Decentraland level of abstraction, for example: new DecentralandMarketplace() (or somethink like that), Make sense give as "utils constants" the addresses of the contracts envolved?

Got it. Makes sense. Yeah the addresses as well as the ABI too.

B)

Sure. I'm wondering if there are others standard contracts that we'll not necessarly have an object like NFT, and maybe give these ABIs to user.

Yes, I think so. I agree that exposing the class with the ABI already inside makes more sense.

C)

Thinking on how Embark and Drizzle do that, both use ganache as dependency. Drizzle uses truffle on the project and point to build/contracts dir Embark do the same (But seems it doesn't use truffle). Summarizing: Both has all stack on the same project.

Ah ok, good to know. Thanks for including this info for comparison. I'll continue to ponder what it means for us.

D)

Our difference here, seems that live on the facts that we are using another device and tasit-contract (truffle) won't be a tasit app dependency. Said that, maybe the key to solve this is prepare the ground on start process (check if ganache is running, get contracts abi and addresses and so on).

tasit-contract could be a tasit app dependency for users that don't want to use the entire SDK but want to use it modularly instead.

And yeah, preparing things on process start sounds like a good approach to me.

E)

Maybe a setup tool/feature that gets ABI from a given contract address (from etherscan).

Yeah, I like the direction you're picturing here. In the tasit repo demo dir? Or in the tasit repo as a a shared tool that all the apps there draw from (maybe the CLI, maybe not)? Or in a shared tool that the demo app uses by having tasit-sdk as a dependency?

F)

Looking what we are doing now from the point of view that we are creating an dapp using TasitSDK.Contract for Decentraland (that we'll support) but it could be another dapp that we'll not support. Maybe we should do the same as user will do (handle with ABI ourselves). On the other hand, since we are tracking these contract already, we can expose that ABIs, or even a Decentraland contracts objects (as a draft from top level of abstraction) but it'll only a subclass from TasitSDK.Contract for now.

Yeah, I'm continuing to think using an ABI in the demo app but then getting the new DecentralandMarketplace() in the decentraland app that is where much of the code from the demo app will eventually move is the way to go.

A and F) Should I implement "draft" Objects for Decentraland Mana Land, Estate and Marketplace?

B) :+1:

C) :+1:

D) I'll try to use the dev user hat to answer these. We'll have two kind of users: 1) We'll use only our TastSDK class In this case, I'm picturing a scenario where the user already has their react-native and truffle projects, assuming I can see these steps: npm i tasit-sdk Start from here on their own.

2) We'll use our demo as scaffolding 2.1) Separetad from truffle project npm i tasit-cli tasit-cli init my-app npm start During the process check if ganche-cli is running and look for deployed contracts to get abi/addresses (and possibly save in a "contracts.json" config file). 2.2) Having trasit-contracts as dependency npm i tasit-cli tasit-cli init my-app npm start As Embark, the developer will work on the same project to write contracts and mobile app. And the start script will run ganache-cli, deploy contracts and then start expo.

E) Maybe if we have some of "contracts.config" for user set contracts addresses for each network we can get ABI automatically under the hood using that etherscan tool.

marcelomorgado commented 5 years ago

Mentioning this tool in case it's useful to think about, since it's kind of on this topic https://github.com/JoinColony/trufflepig

Maybe we have to think on both scenarios:

1. When user under develop process using the SDK (local blockchain / abi from `truffle`/`trufflepig`)

2. When SDK is using to interact with a pre-existing mainnet (without ganache-cli / not deployed by user)

Yeah breaking it down by scenario is helpful, and those seem like two good ones (although I'm not sure how popular TrufflePig is yet).

:+1:

Brainstorm Note: When I was writing item 2, I've thought that we can even suggest to our users development stages when interact with 3rd-party contracts. As we are doing with Decentraland:

Great idea!!!

:+1: Thanks

* Local deployment from same code and test as contract owner

I still think they shouldn't run the tests as the contract owner. They should switch to a different user. Other than that I agree.

:+1:

* Local network fork from testnet (We will do that and test suite should be changed because we'll no longer been owner of contracts);

This could be good for integration tests.

:+1:

* Testnet network (Should be a smooth move from last step)

This would be what we use for testing internally and with alpha testers with TestFlight

:+1:

* Local network fork from mainnet;

* Mainnet network.
  Assuming that the 3rd-party keeps both testnet/mainnet as Decentraland does.

I like the idea of documenting this! And these steps make sense to me. Where do you think these docs belong?

Maybe as Medium post?

ABI Note: What do you think about ethers.js human readable ABI? Should we allows users instante contracts using that?

I like it! I think we should. (Maybe even making our own even simpler abstraction on top of it if appropriate.)

Great, should I create an issue for that?

pcowgill commented 5 years ago

A and F) Should I implement "draft" Objects for Decentraland Mana Land, Estate and Marketplace?

Sure!

1) We'll use only our TastSDK class with a separate repo for contracts, deploying them locally 2) We'll use our demo as scaffolding using the CLI 2.1) Separated from truffle project 2.2) Having tasit-contracts as dependency

Points 1, 2.1, and 2.2 are broken down well and make sense.

It's important that you pointed out that a lot of the users of the SDK will be making front ends for their own contracts. That's a major difference from what we're using the SDK, in the short run (in the medium term we'll have our own contracts too).

I think it's worth splitting 2.2 out into 2.2a where the user won't modify the contracts or tests from tasit-contracts, and 2.2b where the user will modify them.

Note that we shouldn't target all of these use cases at once - just try to make decisions wherever possible while working on one that won't put us down a path that makes the other options harder to enable in the future.

E) Maybe if we have some of "contracts.config" for user set contracts addresses for each network we can get ABI automatically under the hood using that etherscan tool.

I can't totally envision what you're picturing here. Can you explain a bit more?

pcowgill commented 5 years ago

I like the idea of documenting this! And these steps make sense to me. Where do you think these docs belong?

Maybe as Medium post?

Sure! I'll share a Google Doc with you that we can start drafting it in. And maybe at some point we move over to GitBook when our docs get extensive enough and we want it there too.

ABI Note: What do you think about ethers.js human readable ABI? Should we allows users instante contracts using that?

I like it! I think we should. (Maybe even making our own even simpler abstraction on top of it if appropriate.)

Great, should I create an issue for that?

Yes please, thanks!

marcelomorgado commented 5 years ago

A and F) Should I implement "draft" Objects for Decentraland Mana Land, Estate and Marketplace?

Sure!

https://github.com/tasitlabs/TasitSDK/issues/199

  1. We'll use only our TastSDK class with a separate repo for contracts, deploying them locally
  2. We'll use our demo as scaffolding using the CLI 2.1) Separated from truffle project 2.2) Having tasit-contracts as dependency

Points 1, 2.1, and 2.2 are broken down well and make sense.

It's important that you pointed out that a lot of the users of the SDK will be making front ends for their own contracts. That's a major difference from what we're using the SDK, in the short run (in the medium term we'll have our own contracts too).

I think it's worth splitting 2.2 out into 2.2a where the user won't modify the contracts or tests from tasit-contracts, and 2.2b where the user will modify them.

Note that we shouldn't target all of these use cases at once - just try to make decisions wherever possible while working on one that won't put us down a path that makes the other options harder to enable in the future.

:+1: Got it

E) Maybe if we have some of "contracts.config" for user set contracts addresses for each network we can get ABI automatically under the hood using that etherscan tool.

I can't totally envision what you're picturing here. Can you explain a bit more?

For example: config

...
"mainnet": {
  "contracts": {
    "SomeContract": "0x123...890"
  }
}
...

Getting ABI somehow from etherscan, user can use this object "automagically" on code:

const c = new SomeContract();

That "getAbiTool" can be used as solo npm script, on start script and/or on building.

marcelomorgado commented 5 years ago

I like the idea of documenting this! And these steps make sense to me. Where do you think these docs belong?

Maybe as Medium post?

Sure! I'll share a Google Doc with you that we can start drafting it in. And maybe at some point we move over to GitBook when our docs get extensive enough and we want it there too.

:+1:

ABI Note: What do you think about ethers.js human readable ABI? Should we allows users instante contracts using that?

I like it! I think we should. (Maybe even making our own even simpler abstraction on top of it if appropriate.)

Great, should I create an issue for that?

Yes please, thanks!

https://github.com/tasitlabs/TasitSDK/issues/198

pcowgill commented 5 years ago

For example: config

...
"mainnet": {
"contracts": {
"SomeContract": "0x123...890"
}
}
...

Getting ABI somehow from etherscan, user can use this object "automagically" on code:

const c = new SomeContract();

That "getAbiTool" can be used as solo npm script, on start script and/or on building.

@marcelomorgado This sounds good!