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 neth #520

Merged
merged 21 commits into from
Nov 7, 2022
Merged

add neth #520

merged 21 commits into from
Nov 7, 2022

Conversation

mattlockyer
Copy link
Contributor

@kujtimprenkuSQA changes have been made.

NETH repo for development, docs and NETH install page:
https://github.com/NearDeFi/neth/

neth.app live NETH installation page.

packages/neth/README.md Outdated Show resolved Hide resolved
packages/neth/README.md Outdated Show resolved Hide resolved
packages/neth/README.md Outdated Show resolved Hide resolved
@mattlockyer mattlockyer requested a review from DamirSQA October 24, 2022 21:24
@DamirSQA
Copy link
Contributor

DamirSQA commented Oct 25, 2022

Hi @mattlockyer , I'm experiencing issues when trying to use NETH, so I hope you can help me with.

When I try to connect with https://neth.app/?network=testnet or https://neardefi.github.io/neth/?network=testnet, I'm experiencing errors.

image

Similar goes when I'm using wallet-selector from https://neardefi.github.io/guest-book-wallet-selector/

image

If I missed something, wouldn't mind some pointers. Thanks!

@mattlockyer
Copy link
Contributor Author

If I missed something, wouldn't mind some pointers. Thanks!

Sorry you didn't miss anything. I have the Aurora network added and I think there were some changes in the errors thrown by @metamask/detect-provider

I have updated the live install site and the code here to reflect and catch the correct error and prompt a network switch in MetaMask.

Please try again. Thanks.

@mattlockyer
Copy link
Contributor Author

Hi @DamirSQA were you able to install NETH yet and test this implementation?

@DamirSQA
Copy link
Contributor

Hi @mattlockyer thanks for the updates.
I'm sharing screen recording of an account creation flow, where you can take a look on the issue I've experienced.
https://www.loom.com/share/ceaddec1b4104aaca724006c7fdca2b9

In the wallet list, NETH is missing, and colleague of mine is missing it from the live examples so you might want to investigate that.

@mattlockyer
Copy link
Contributor Author

You need to get an app key to use apps.

First complete this step by clicking the final button on the NETH install page.

image

@DamirSQA
Copy link
Contributor

DamirSQA commented Oct 26, 2022

Thanks @mattlockyer, that did the trick.

In regard to Neth missing from a list, below is the case of the example page when the wallet extension is missing.

image

You can check out some other wallets, so that in a same way when metamask is missing, users can be aware of and directed to the metamask download page.

image

On localhost, NETH is missing from the list regardless if wallet extension is present or not, so it would be great if that can be addressed.

@mattlockyer
Copy link
Contributor Author

Thanks @mattlockyer, that did the trick.

In regard to Neth missing from a list, below is the case of the example page when the wallet extension is missing.

Great feedback. Let me work on this case and I'll request a review tomorrow.

@mattlockyer
Copy link
Contributor Author

@DamirSQA the visibility of NETH and prompt to install should be working now. Thanks for pointing this out. Not often am I browsing without a Web3 wallet 😄

@DamirSQA
Copy link
Contributor

Hi @mattlockyer , I'm afraid that we still have the same issue. Might be that the commit is missing, nevertheless I've recorded a short example for you, so you can check it out on link below.

wallet not installed example

@max-mainnet
Copy link

@mattlockyer
Copy link
Contributor Author

Hi @mattlockyer , I'm afraid that we still have the same issue. Might be that the commit is missing, nevertheless I've recorded a short example for you, so you can check it out on link below.

Thanks for the video. I understood the problem.

There was an issue with the gh-pages not picking up the new build. I manually triggered another build and it seems to be working now.

Please try again @DamirSQA

@mattlockyer
Copy link
Contributor Author

https://github.com/NearDeFi/wallet-selector/blob/neth/packages/neth/src/lib/neth-lib.js#L735-L749 Hi, @mattlockyer , found my receiver_id replaced by RECEIVER_MARKER

This is to protect against injections and swapping of receiver_id / account_id in known standards patterns

@DamirSQA
Copy link
Contributor

Thanks @mattlockyer, looking great on gh-pages, only PR left to be updated.

@mattlockyer
Copy link
Contributor Author

Thanks @mattlockyer, looking great on gh-pages, only PR left to be updated.

Is this the update you're referring to?

dfc678f

@max-mainnet
Copy link

However, as sometimes we do need a receiver_id in ft_transfer_call args, is there any ways to let my receiver_id or account_id to take effect, because I found any time in this functioncall, it is replaced by your maker.

@DamirSQA
Copy link
Contributor

DamirSQA commented Oct 31, 2022

Is this the update you're referring to?

dfc678f

I've recorded video with modal-ui-js example, but it's the same for modal-ui. You can check app.component.ts and WalletSelectorContext.tsx and you'll see that Neth is not initialized.

You should add this in tsconfig.base.json

       "@near-wallet-selector/neth": [
           "packages/neth/src/index.ts" 
        ]

and then you can import { setupNeth } from "@near-wallet-selector/neth";
in app.component.ts and WalletSelectorContext.tsx

Also is it possible to convert neth-lib.js to neth-lib.ts ?

packages/neth/package.json Outdated Show resolved Hide resolved
@DamirSQA
Copy link
Contributor

DamirSQA commented Nov 2, 2022

Hi @mattlockyer , looks like the merge isn't correct in WalletSelectorContext.tsx and app.component.ts, check it please, also you can check the pointers below.

  • You should update root readme file to include Neth too, and also if the setup requires extra/custom params, update the docs/readme to include the info (useModalCover and gas) and explain the types and what each property is needed for.
  • Make the deprecated state configurable from the setupNeth, you can check in other wallets.
  • .gitignore needs to be removed

In signAndSendTransaction inside the action you can pass the gas property on the dApp side.
https://github.com/near/wallet-selector/blob/main/examples/react/components/Content.tsx#L17
https://github.com/near/wallet-selector/blob/main/examples/react/components/Content.tsx#L121

What the "gas" amount is being used for in the setupNeth?

@mattlockyer
Copy link
Contributor Author

What the "gas" amount is being used for in the setupNeth?

Please see this explanation:
#520 (comment)

The default gas for all NETH transactions (if multiple are bundled) is 200 Tgas (plenty for most apps)

If the app wants to override this global gas amount they can pass that in to setupNeth.

NETH can perform multiple transactions in a single NEAR transaction, otherwise the user must sign multiple times, once for each transaction.

I may have to add a configuration option that prevents NETH from bundling transactions into a single NEAR transactions since there seems to be use cases where signing individually is advantageous.

@DamirSQA
Copy link
Contributor

DamirSQA commented Nov 3, 2022

Hey @mattlockyer , thanks for the updates.
I've tested in React, it's looking great, however when I try to serve Angular, it has errors and it fails to compile, so you might want to address that.

Regarding root readme you can check it out here
https://github.com/near/wallet-selector/blob/dev/README.md?plain=1#L1

What's left is to adapt SetupNeth to have depricated as an input parameter, and to remove .gitignore from neth folder.

Other than mentioned, is there anything else you might want to change?

examples/react/contexts/WalletSelectorContext.tsx Outdated Show resolved Hide resolved
packages/neth/README.md Outdated Show resolved Hide resolved
@mattlockyer
Copy link
Contributor Author

@DamirSQA fixed the angular example, please try it out.

Bundle logic was updated this morning to allow the user to sign everything first and then attempt to process all transactions once signatures are all confirmed by user.

This fits the model that most dapp developers are used to when they call signAndSendTransactions

The user accepts all transactions by signing everything, or none at all.

examples/angular/tsconfig.json Outdated Show resolved Hide resolved
packages/neth/src/lib/neth.ts Show resolved Hide resolved
@DamirSQA
Copy link
Contributor

DamirSQA commented Nov 4, 2022

Hey @mattlockyer , we're planning a release next week, so in order for neth to be included, it would be great if you can address the rest of the issues ( TS, depricated, readme ). After release if you make additional changes, new PR can be raised.

Also when you're finished with the changes, you can merge dev to your branch and fix conflicts if needed.

@mattlockyer
Copy link
Contributor Author

@DamirSQA we should be good for the major release. Have been testing with some apps and cross contract calls to Aurora.

@DamirSQA DamirSQA merged commit f4ec3d6 into near:dev Nov 7, 2022
@DamirSQA DamirSQA mentioned this pull request Nov 7, 2022
@kujtimprenkuSQA
Copy link
Contributor

Hey, @mattlockyer.

We have been testing Wallet Selector with the latest version of near-api-js and while testing it out we noticed that signAndSendTransaction(s) has stopped working for NETH we are able to signIn and signOut successfully.

neth-transaction

The easiest way to reproduce this is to clone/fork the wallet-selector project and update the version of near-api-js version to 1.1.0 and then build all the packages with yarn build:all after that serve either the react or angular example.

@mattlockyer
Copy link
Contributor Author

Will have a look in the morning. It's likely something small re: client side preparation of the transactions. I'll take care of it ASAP! Thank you for flagging!

@mattlockyer mattlockyer mentioned this pull request Nov 14, 2022
@mattlockyer
Copy link
Contributor Author

mattlockyer commented Nov 14, 2022

#557

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.

None yet

4 participants