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(transfer): javascript playground #624

Draft
wants to merge 26 commits into
base: main
Choose a base branch
from

Conversation

jessevanmuijden
Copy link
Contributor

@jessevanmuijden jessevanmuijden commented Jul 19, 2023

Ticket

This PR updates the depency on @kadena/client to the latest (~1.0.0) version. Also, it extends the module explorer functinoality with a JavaScript playground. It should be considered if the module explorer extension should better be moved to another PR and limit the current PR to just the kadena client version update.

The approach to the module explorer and the JavaScript playground is as follows.

Routing
Users can search modules on any chain by typing in the search field. Below the form, a list of modules in the selected the chain are displayed, filtered on the search query. Clicking on any module name will navigate the user to a /module-explorer/networks/{network}/chains/{chain/modules/{moduleName} route. On the respective page, the pact code as well as a list of functions, capabilities and interfaces are displayed. Clicking on a function name will navigate the user to a /module-explorer/networks/{network}/chains/{chain/modules/{moduleName}/functions/{function} route. This page contains a code editor and a readonly output frame.

Evaluating user provided JavaScript
A user can enter JavaScript code in the editor and click the "Run" button to execute the JavaScript. the scopedEval npm package is used to execute the user provided JavaScript within the scope of the application. For security purposes, only { Pact, getClient, signWithChainweaver, isSignedCommand } from the @kadena/client as well as a simple Logger to display output are available in the scope where the user provided JavaScript is executed in. This approach does require a thorough evaluation with regard to security.

Example code snippets
The codebase contains many example code snippets, e.g. in the cookbook and the client-examples directory. It could help the onboarding of new developers on the Kadena platform, if we could provide example code snippets for use in the aforementioned JavaScript editor. This would require a way to match example code snippets to the function that someone wants to use. As a proof of concept I have created a git repository on my own github account with a directory structure that mirrors the routing of the module explorer described above. If we agree on this approach, we should probably find a good central location for the example code snippets in this kadena repository. To give an example of how it works, there is an example script for the get-balance function of the coin module on chain 9 of TESTNET located in /networks/TESTNET/chains/9/modules/coin/functions/get-balance.js. If the transfer app can find this file, it will show a button "Load example code". When it is clicked, the example code from the git repository will be loaded into the code editor and can be executed by clicking the "Run" button. It could also be an idea to turn the get-balance.js file into a ./get-balance directory containing multiple example snippets for the function at hand. Maintainability of this approach needs to be discussed, because it may not be practical to maintain example snippets separately for every chain in case a Pact module is deployed on all chains. On the other hand, A module with the same name may have a different implementation on another chain.

to do

  • Unit tests for services in the transfer app
  • Extend unit tests for components in the transfer app
  • Integration tests for transfer app functionality
  • E2E tests for transfer app
  • Add tests for packages that were upgraded to kadena/client v1
Scherm­afbeelding 2023-07-19 om 07 30 31

@vercel
Copy link

vercel bot commented Jul 19, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

3 Ignored Deployments
Name Status Preview Comments Updated (UTC)
alpha-docs ⬜️ Ignored (Inspect) Visit Preview Jul 25, 2023 0:43am
react-ui ⬜️ Ignored (Inspect) Visit Preview Jul 25, 2023 0:43am
transfer ⬜️ Ignored (Inspect) Jul 25, 2023 0:43am

@jessevanmuijden jessevanmuijden force-pushed the feat/transfer/javascript-playground branch from 9587a5a to bffeff4 Compare July 20, 2023 21:34
@jessevanmuijden jessevanmuijden force-pushed the feat/transfer/javascript-playground branch from 5c39a8d to 5daf4b1 Compare July 21, 2023 09:47
@jessevanmuijden jessevanmuijden marked this pull request as ready for review July 21, 2023 16:08
@jessevanmuijden jessevanmuijden force-pushed the feat/transfer/javascript-playground branch from 85033bc to acfe6d4 Compare July 24, 2023 10:18
@nil-amrutlal
Copy link
Contributor

nil-amrutlal commented Jul 24, 2023

There are some conflicting files. Can we resolve these and try again? From checking the screenshot @sstraatemans provided (see below) I cant really see the problem. Could have to do with the versioning of the client

image

@jessevanmuijden jessevanmuijden force-pushed the feat/transfer/javascript-playground branch from cd3b6bc to 35d679b Compare July 25, 2023 12:39
Comment on lines 15 to +29
export interface IAceEditorProps {
code?: string;
width?: string;
height?: string;
readonly?: boolean;
onChange?: IOnchange;
}

const AceViewerComponent: FC<IAceEditorProps> = ({ code }) => (
const AceViewerComponent: FC<IAceEditorProps> = ({
code,
width,
height,
readonly,
onChange,
}) => (
Copy link
Member

Choose a reason for hiding this comment

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

I know Ace Editor has support for Vim-mode. Can we enable that with a toggle? Not blocking for this PR, just a thought

Copy link
Member

@alber70g alber70g left a comment

Choose a reason for hiding this comment

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

Minor comments. Only code review, not execution

Comment on lines +3 to +5
const client = getClient();

export default client;
Copy link
Member

Choose a reason for hiding this comment

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

I think it's preferred to use named exports. It's easier to resolve a missing import that way. Also making it a bit more specific might help to distinguish from other clients.

Suggested change
const client = getClient();
export default client;
export const kadenaClient = getClient();

Comment on lines +71 to +75
const result = (await fundExistingAccount(
accountName,
chainID,
AMOUNT_OF_COINS_FUNDED,
)) as IFundExistingAccountResponse;
Copy link
Member

Choose a reason for hiding this comment

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

Can't this type be set in the fundExistingAccount function?

Comment on lines +21 to +23

const FunctionPage: FC = () => {
Debug('kadena-transfer:pages:transfer:module-explorer:module:function');
Copy link
Member

Choose a reason for hiding this comment

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

How is this being used?
Usually you assign the return value to a variable to use in other places.
This can be assigned outside the component as it doesn't change in other components

Suggested change
const FunctionPage: FC = () => {
Debug('kadena-transfer:pages:transfer:module-explorer:module:function');
const log = Debug('kadena-transfer:pages:transfer:module-explorer:module:function');
const FunctionPage: FC = () => {
log('executing FunctionPage'); // example

import React, { FC, useEffect, useState } from 'react';

const ModulePage: FC = () => {
Debug('kadena-transfer:pages:transfer:module-explorer:module');
Copy link
Member

Choose a reason for hiding this comment

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

Idem

Comment on lines 70 to +77
const apiHost = getKadenaConstantByNetwork('TESTNET').apiHost({
networkId: NETWORK_ID,
chainId,
});

await transactionBuilder.send(apiHost);
transaction.sigs = [{ sig: signature1.sig }, { sig: signature2.sig }];

return await transactionBuilder.pollUntil(apiHost, {
onPoll,
});
const { submit, pollStatus } = getClient(apiHost);
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this work with WalletConnect? Take the network from the walletconnect settings, this will open up using this on the devnet as well.
Might be better to do in a next PR though

Comment on lines +41 to +44
return await local(transaction, {
preflight: false,
signatureVerification: false,
});
Copy link
Member

Choose a reason for hiding this comment

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

You can use dirtyRead from getClient

@@ -26,27 +25,29 @@ export const listModules = async (
ttl: number = kadenaConstants.API_TTL,
): Promise<IModulesResult> => {
debug(listModules.name);
Copy link
Member

Choose a reason for hiding this comment

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

idem

Comment on lines +42 to +45
const response = await local(transaction, {
preflight: false,
signatureVerification: false,
});
Copy link
Member

Choose a reason for hiding this comment

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

Idem

Comment on lines +28 to +31
const response = await local(transaction, {
preflight: false,
signatureVerification: false,
});
Copy link
Member

Choose a reason for hiding this comment

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

Idem

Comment on lines +28 to +31
const response = await local(transaction, {
preflight: false,
signatureVerification: false,
});
Copy link
Member

Choose a reason for hiding this comment

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

Idem

alber70g

This comment was marked as duplicate.

Copy link
Member

@alber70g alber70g left a comment

Choose a reason for hiding this comment

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

Minor comments. Only code review, not execution

Copy link
Contributor

This PR is stale because it is open for 60 days with no activity

@Randynamic
Copy link
Contributor

@javadkh2 @alber70g research what to do with this PR

@Randynamic Randynamic marked this pull request as draft September 4, 2024 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants