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

feat: add ContractView using contract-builder #24

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

davidyuk
Copy link
Member

depends on #23 aeternity/contract-builder#8

This PR is supported by the Æternity Crypto Foundation

@davidyuk davidyuk force-pushed the feature/use-contract-builder branch from 3c43898 to a54bb7a Compare April 8, 2023 06:23
@davidyuk davidyuk requested review from thepiwo and marc0olo April 8, 2023 06:24
Copy link
Contributor

@marc0olo marc0olo left a comment

Choose a reason for hiding this comment

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

I appreciate in general to introduce contract interaction into the boilerplate. But I have very strong opinion on:

  • Avoiding usage of http compiler (will be dropped and not usable anymore in the future)
  • Avoid using source code in a frontend application
    • A contract could be deployed on testnet already. Only the ACI and the contract address would be required then (this is the approach to advertise for frontend developers). Of course the README should then explain which contract is being used and how the bytecode and ACI has been generated
  • I think the contract builder should be showcased independently
  • If we really want to showcase "on-demand" deployment of a contract in a frontend (which I advise against and prefer usage of contract factories), the bytecode should exist already and not be compiled in the context of the frontend boilerplate

@marc0olo marc0olo requested a review from dincho April 11, 2023 15:12
@marc0olo
Copy link
Contributor

I added @dincho as he might also have some opinion I would like to hear :)

@davidyuk davidyuk mentioned this pull request Apr 22, 2023
@davidyuk
Copy link
Member Author

davidyuk commented Apr 22, 2023

Avoiding usage of http compiler

No problem, I've reverted the last commit. By default contract-builder uses aesophia_cli.

A contract could be deployed on testnet already.

This case is supported currently, Multiplier accepts address option. I can add an option to contract-builder to don't generate bytecode, but I doubt it worth it, because bytecode doesn't take too much space usually (it is limited by block size, I've got the max of 293kb). Even if contract already deployed, bytecode can be used to verify on-chain contract.

Only the ACI and the contract address would be required then

In case of typescript, developer need not only ACI, but a type of contract interface. Currently, he needs to write it manually, to avoid this I'm planning to implement it in contract-builder.

Using contract-builder developer don't need to know about ACI at all, it should make onboarding easier. Also, this approach would work the best for prototyping, when developer need to build UI and contract simultaneously.

I think the contract builder should be showcased independently

It has own examples in the repo. As I see contract-builder should be a straightforward approach to integrate contacts into aepps, this is why I'm proposing to add this to boilerplate.

contract factories

Do you mean when the bytecode of already deployed contract gets cloned? Contract deployment is relatively cheap now (I've paid 0.0056ae on mainnet to deploy 293kb bytecode explorer), so this optimisation doesn't seem to be very important, though it should be supported aeternity/aepp-sdk-js#1798.

the bytecode should exist already and not be compiled in the context of the frontend boilerplate

Not sure about this. Contract bytecode is a build artifact. If build dependencies (compiler version) defined correctly it would be the same between builds. Not necessary to store the bytecode explicitly if it can be generated on demand.

@marc0olo
Copy link
Contributor

This case is supported currently, Multiplier accepts address option.

Then let's use it and I think I am fine :-) Bytecode should not be required by a typical frontend (which isn't an online IDE like aestudio or the contracts editor)

Even if contract already deployed, bytecode can be used to verify on-chain contract.

Bytecode verification via frontend is kind of an illusion. I don't think it is the right place to handle it. Have discussed this several times with @dincho and @thepiwo. There is some contrary opinions here afaik.

In case of typescript, developer need not only ACI, but a type of contract interface. Currently, he needs to write it manually, to avoid this I'm planning to implement it in contract-builder.

ok, I was not aware of this. definitely makes sense to have a library that generates this contract interface.

still I am wondering if it would be better to have this as a separate step in development and then just add the artifacts (ACI & contract interface) to the frontend.

for prototyping and simultaneous development of contract and UI it is definitely cool :)

It has own examples in the repo. As I see contract-builder should be a straightforward approach to integrate contacts into aepps, this is why I'm proposing to add this to boilerplate.

still not sure about this one, but I now understand your thoughts better.

Do you mean when the bytecode of already deployed contract gets cloned? Contract deployment is relatively cheap now (I've paid 0.0056ae on mainnet to deploy 293kb bytecode explorer), so this optimisation doesn't seem to be very important, though it should be supported aeternity/aepp-sdk-js#1798.

I just mean that ideally the frontend never has to know about the bytecode. it just doesn't need bytecode. if the UI should deploy a smart contract it would be better to have a contract on-chain which uses contract factories to create new contracts. at last I personally would always prefer this approach :-)

I commented the issue you created. can be closed IMO as this is independent of the SDK.

If build dependencies (compiler version) defined correctly it would be the same between builds. Not necessary to store the bytecode explicitly if it can be generated on demand.

generally agree. but still we shouldn't need bytecode at all in the UI in 99,99% of applications.

Comment on lines 34 to 44
if (process.env.VUE_APP_WALLET_SECRET_KEY) {
const account = new MemoryAccount(process.env.VUE_APP_WALLET_SECRET_KEY)

await client.addAccount(account, {select: true})
sdk = client
// AeSdk instance can't be in deep reactive https://stackoverflow.com/a/69010240
aeSdk.value = shallowReactive(new AeSdk({
...aeSdkOptions,
accounts: [account],
}))

walletStatus.value = 'connected'
await fetchWalletInfo(client)
await fetchAccountInfo()
} else {

Choose a reason for hiding this comment

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

I am not sure if this is really required. Examples should be short and precise. Memory account wallet example in real life is not feasible. A wallet for every AEPP doesn't make sense. I believe developers are looking for a Browser extension wallet connection from their AEPPs. If a Static wallet example is required then we can provide one more example repository by keeping both projects minimalistic.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is out of scope of this PR, I' would discuss it in a new issue. As I understand, MemoryAccount is used for development, to don't confirm connection to wallet each time 🙂 or running in e2e test. Old versions of cypress wasn't compatible with extensions.

Choose a reason for hiding this comment

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

If it is for development then it's better to choose the right environment variable rather than introduce a new one
Intead process.env.VUE_APP_WALLET_SECRET_KEY of use process.env.NODE_ENV. Perhaps a few lines about this on README would be helpful.

@davidyuk davidyuk force-pushed the feature/use-contract-builder branch from e1e6558 to 5590f25 Compare April 27, 2023 03:34
@davidyuk
Copy link
Member Author

davidyuk commented Apr 27, 2023

@marc0olo I'm fine to don't include bytecode by default in the next version of contract builder aeternity/contract-builder#12
Can we merge this as it is right now?

I wouldn't remove bytecode completely because contract-builder may be used in nodejs apps as well 🤔

@marc0olo
Copy link
Contributor

@marc0olo I'm fine to don't include bytecode by default in the next version of contract builder aeternity/contract-builder#12
Can we merge this as it is right now?

command npm run serve fails right now, probably due to usage of aesophia_cli:

> [email protected] serve
> npm run serve:testnet


> [email protected] serve:testnet
> vue-cli-service serve --mode development

 INFO  Starting development server...


 ERROR  Failed to compile with 1 error                                                                                                                                                               8:54:26 AM

 error  in ./src/utils/aeternity/contracts/Multiplier.aes

Syntax Error: Error: Command failed: escript /home/marc0olo/git/aeternity/aepp-boilerplate-vue/node_modules/@aeternity/aepp-sdk/bin/aesophia_cli --version


ERROR in ./src/utils/aeternity/contracts/Multiplier.aes
Module build failed (from ./node_modules/@aeternity/contract-builder/dist/loader.js):
Error: Command failed: escript /home/marc0olo/git/aeternity/aepp-boilerplate-vue/node_modules/@aeternity/aepp-sdk/bin/aesophia_cli --version

    at ChildProcess.exithandler (node:child_process:398:12)
    at ChildProcess.emit (node:events:527:28)
    at maybeClose (node:internal/child_process:1092:16)
    at Socket.<anonymous> (node:internal/child_process:451:11)
    at Socket.emit (node:events:527:28)
    at Pipe.<anonymous> (node:net:709:12)
 @ ./node_modules/babel-loader/lib/index.js??clonedRuleSet-40.use[0]!./node_modules/vue-loader/dist/index.js??ruleSet[0].use[0]!./src/views/ContractView.vue?vue&type=script&setup=true&lang=js 3:0-69 11:27-37 54:15-25
 @ ./src/views/ContractView.vue?vue&type=script&setup=true&lang=js 1:0-215 1:0-215 1:216-420 1:216-420
 @ ./src/views/ContractView.vue 2:0-74 3:0-69 3:0-69 8:49-55
 @ ./src/router/index.js 13:19-89
 @ ./src/main.js 3:0-30 4:19-25

webpack compiled with 1 error

despite not being able to test the application right now, I see in the code, that the boilerplate still contains the deploy functionality which I think should be removed.

I wouldn't remove bytecode completely because contract-builder may be used in nodejs apps as well 🤔

yeah but that is a different context and might be a different boilerplate then. and in many cases I suspect the nodejs backend also wouldn't deploy new contracts. still in context of contract-builder it is useful to let it generate bytecode, agree. better to further discuss this in aeternity/contract-builder#12

@davidyuk
Copy link
Member Author

davidyuk commented May 2, 2023

probably due to usage of aesophia_cli

Yea, you need Erlang/OTP (version 23 or newer) installed

that the boilerplate still contains the deploy functionality which I think should be removed

How would you showcase contract interactions then, should it call an already deployed contract on testnet? Would be nice to include a way to alter the contract, so to provide a separate deploy script or keep it as it is now.

@marc0olo
Copy link
Contributor

marc0olo commented May 2, 2023

Yea, you need Erlang/OTP (version 23 or newer) installed

should be added to the README then. I would prefer to make this step optional if possible

How would you showcase contract interactions then, should it call an already deployed contract on testnet? Would be nice to include a way to alter the contract, so to provide a separate deploy script or keep it as it is now.

exactly, this is what the UI is doing in 99,99% of all usecases, right? I do not see a lot of usecases apart from local development where you'd use the UI to deploy contracts. and if the UI is used, the already deployed contract should make use of contract factories to deploy new instances. from UI perspective it would then be a simple entrypoint call to an already deployed contract again.

the only thing I am wondering is the type generation for contracts which would require the sophia compiler 🤔 separate instructions for this would be good I think. maybe in this case it is really better to commit generated types which could be overwritten if Erlang/OTP is installed and a specific script is executed.

the boilerplate should work for everybody without having to install Erlang/OTP. if we really wanna showcase it, I would prefer to make it optional and provide according instructions.

@thepiwo thepiwo removed their request for review February 22, 2024 07:21
@dincho dincho removed their request for review March 13, 2024 11:25
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