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

Add evm function call #270

Merged
merged 69 commits into from
Sep 10, 2024
Merged

Add evm function call #270

merged 69 commits into from
Sep 10, 2024

Conversation

Pessina
Copy link
Collaborator

@Pessina Pessina commented Jul 19, 2024

This PR includes:

  • Add support for EVM Function Call using ChainSignatures
  • Add local EVM blockchain using hardhat for testing purpose
  • Fix Webkit bug on Submit button

Obs: The tests for this PR are expected to be flaky as they depend on relayer and ChainSignature contract that fail frequently. The solution would be run those dependencies locally (I can setup this later)

@@ -0,0 +1,92 @@
// Origal repo: https://github.com/Consensys/Tokens/blob/master/contracts/eip20/EIP20.sol
Copy link
Collaborator

Choose a reason for hiding this comment

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

so this file is extended version of file from above link? could you add a bit more detail about what has modified/extended and why? (eg. I notice that mint doesn't exist on above, but added below)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't added any explanation because we will never change this file again. It's just a contract implementing the ERC20 interface for testing.

The code here was copy pasted from the URL linked, and the mint function was added so we can distribute the tokens as we want for testing.

I was checking other options to implement this contracts, looks like Open Zeppelin haves a npm package with the implementations for all we need ERC20, ERC721, ERC1155.

But if it's ok I would like to tackle this in another PR, there is too much work here already.

That's the package I'm looking to use: https://www.npmjs.com/package/@openzeppelin/contracts

const isUserAuthenticated = await getAuthState();
if (isUserAuthenticated !== true) {
onError('You are not authenticated or there has been an indexer failure');
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it ok to exclude isUserAuthenticated check?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I moved it to here:

We need to be authenticated before estimate the gas for the function call.

Copy link
Collaborator

@hcho112 hcho112 left a comment

Choose a reason for hiding this comment

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

This is amazing work. Left few comments. Would prefer to get more reviews from rest of the team

@race-of-sloths
Copy link

❌ PR inactive

This PR has been inactive for two weeks. It's now marked as stale.

Tip

If you think it's a mistake, you can restart the bot with include command to proceed 🔄

maxFeePerGas: yup.mixed<bigint>().optional(),
maxPriorityFeePerGas: yup.mixed<bigint>().optional(),
gasLimit: yup.mixed<bigint>().optional(),
export const SendEVMMultichainMessageSchema = BaseSendMultichainMessageSchema.shape({
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should also validate the chainId like before. Also are we no longer supporting dapps specifying the gas themselves and only fetching when it's not specified?

Copy link
Collaborator Author

@Pessina Pessina Aug 28, 2024

Choose a reason for hiding this comment

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

The chainId it's being validate on the derivationPath.

For the other properties, the type mixed accepts anything, but you are right we should narrow the validation. I will improve it

Copy link
Collaborator

Choose a reason for hiding this comment

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

What about EVM chain previously validated here:

chainId: yup.lazy((value) => yup.mixed<bigint>().test(
    'is-valid-chainId',
    `Invalid chainId value: ${value}`,
    (val) => [BigInt(1), BigInt(56), BigInt(97),

// Check if both are present or both are undefined
if ((fee && utxos) || (!fee && !utxos)) {
return true; // Validation succeeds
transaction: yup.object().shape({
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should validate that a from is provided here. As discussed the dapp can choose to be explicit about the from and if it is provided the signer should check that it matches the derivation path

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The BTC doesn't accept a from, even if provided it will be ignored, now and before.

The EVM can accept a from and as discussed it will be validated on the multichain-tools, here is the place where the validation occurs: https://github.com/near/multichain-tools/blob/cce661c13a53b8b89dde5f3c82f3aadd1790a0aa/src/chains/EVM/EVM.ts#L156

maxPriorityFeePerGas: yup.mixed<bigint>().optional(),
gasLimit: yup.mixed<bigint>().optional(),
export const SendEVMMultichainMessageSchema = BaseSendMultichainMessageSchema.shape({
transaction: yup.mixed().required(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since it's no longer validated in the base schema, we should make sure that the dev has passed in a to, value and optionally a from here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, it will be fixed when we narrow down the schema

Copy link
Collaborator

@esaminu esaminu left a comment

Choose a reason for hiding this comment

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

LGTM. Great work @Pessina !

@Pessina Pessina merged commit df38c9d into main Sep 10, 2024
4 checks passed
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.

4 participants