wallet-selector icon indicating copy to clipboard operation
wallet-selector copied to clipboard

add neth

Open mattlockyer opened this issue 2 years ago • 15 comments

@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.

mattlockyer avatar Oct 21 '22 16:10 mattlockyer

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!

DamirSQA avatar Oct 25 '22 13:10 DamirSQA

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 avatar Oct 25 '22 16:10 mattlockyer

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

mattlockyer avatar Oct 26 '22 14:10 mattlockyer

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.

DamirSQA avatar Oct 26 '22 15:10 DamirSQA

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

mattlockyer avatar Oct 26 '22 15:10 mattlockyer

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.

DamirSQA avatar Oct 26 '22 22:10 DamirSQA

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 avatar Oct 26 '22 22:10 mattlockyer

@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 :smile:

mattlockyer avatar Oct 27 '22 15:10 mattlockyer

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

DamirSQA avatar Oct 28 '22 10:10 DamirSQA

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

max-mainnet avatar Oct 28 '22 11:10 max-mainnet

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 avatar Oct 28 '22 14:10 mattlockyer

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

mattlockyer avatar Oct 28 '22 15:10 mattlockyer

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

DamirSQA avatar Oct 28 '22 15:10 DamirSQA

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

Is this the update you're referring to?

https://github.com/near/wallet-selector/pull/520/commits/dfc678f7dfb789f7844f486dc285539a2b2e4873

mattlockyer avatar Oct 28 '22 16:10 mattlockyer

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.

max-mainnet avatar Oct 29 '22 07:10 max-mainnet

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 ?

DamirSQA avatar Oct 31 '22 10:10 DamirSQA

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.

When the contract executes the transaction these should all be replaced again with their respective account_ids.

Have you tested it? Do you have a transaction I can see or some code? It's hard to understand if you have a problem or just a question without more context.

mattlockyer avatar Nov 01 '22 13:11 mattlockyer

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.

When the contract executes the transaction these should all be replaced again with their respective account_ids.

Have you tested it? Do you have a transaction I can see or some code? It's hard to understand if you have a problem or just a question without more context.

Sorry my misunderstanding on it, I apologize. just another issue about gas limit, https://github.com/NearDeFi/wallet-selector/blob/dfc678f7dfb789f7844f486dc285539a2b2e4873/packages/neth/src/lib/neth-lib.js#L39, kind of constraint for multi transactions hard-coded to 200T gas limit,

max-mainnet avatar Nov 01 '22 13:11 max-mainnet

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.

Sorry for the confusion. I updated the examples and tested them.

This was helpful since I found an issue in the NETH transaction signing when you do not include receiverId. This has been fixed now.

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

I can look into it but there's a lot of transaction validation / response checking that seems to have issues with the types used by wallet-selector.

Is this a deal breaker?

mattlockyer avatar Nov 01 '22 15:11 mattlockyer

kind of constraint for multi transactions hard-coded to 200T gas limit

Good point. I added an option to customize the gas amount.

https://github.com/near/wallet-selector/pull/520/commits/4fd91ff4ea02bae4b8700914392e141bac04881e

This is done in the setupNeth({ ..., gas, ...}) call.

It's global for all transactions fater you call setup.

Since wallet-selector methods e.g. signAndSendTransaction(s) do not have gas as an argument, I'm not sure how other wallets are modifying gas or if there's a best practice?

@DamirSQA :point_up:

mattlockyer avatar Nov 01 '22 17:11 mattlockyer

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

I can look into it but there's a lot of transaction validation / response checking that seems to have issues with the types used by wallet-selector.

Is this a deal breaker?

Hey @mattlockyer appreciate your effort. It would be great if you can take a look into, but if there are issues, it can stay in js.

Also I'd like to ask you to apply dev changes to your branch.

DamirSQA avatar Nov 01 '22 17:11 DamirSQA

kind of constraint for multi transactions hard-coded to 200T gas limit

Good point. I added an option to customize the gas amount.

4fd91ff

This is done in the setupNeth({ ..., gas, ...}) call.

It's global for all transactions fater you call setup.

Since wallet-selector methods e.g. signAndSendTransaction(s) do not have gas as an argument, I'm not sure how other wallets are modifying gas or if there's a best practice?

@DamirSQA ☝️

Thanks a lot, I think it improves in some ways, would you please consider the gas limit of 300T in one functioncall, and a case when there are many transactions which share this 300T as your code shows https://github.com/max-mainnet/wallet-selector/blob/neth/packages/neth/src/lib/neth-lib.js#L990-L1013, hope I don't get into somewhere wrong,

I have tried in your repo and started to serve react. https://explorer.testnet.near.org/transactions/FMHavm2c6NK4i37gaR6PzS8h5Says48UyhJSRmnvim3A, which I tried to sign and send 20 transactions ( push 20 transactions when click multiple transactions option in guest book ), it throw gas error; and if I was to increase the customized gas option to more than 300T, I can't even send those transactions.

sorry to bother again,

max-mainnet avatar Nov 01 '22 18:11 max-mainnet

kind of constraint for multi transactions hard-coded to 200T gas limit

Good point. I added an option to customize the gas amount. 4fd91ff This is done in the setupNeth({ ..., gas, ...}) call. It's global for all transactions fater you call setup. Since wallet-selector methods e.g. signAndSendTransaction(s) do not have gas as an argument, I'm not sure how other wallets are modifying gas or if there's a best practice? @DamirSQA ☝️

Thanks a lot, I think it improves in some ways, would you please consider the gas limit of 300T in one functioncall, and a case when there are many transactions which share this 300T as your code shows https://github.com/NearDeFi/wallet-selector/blob/neth/packages/neth/src/lib/neth-lib.js#L990-L1013, hope I don't get into somewhere wrong,

I have tried in your repo and started to serve react. https://explorer.testnet.near.org/transactions/FMHavm2c6NK4i37gaR6PzS8h5Says48UyhJSRmnvim3A, which I tried to sign and send 20 transactions ( push 20 transactions when click multiple transactions option in guest book ), it throw gas error; and if I was to increase the customized gas option to more than 300T, I can't even send those transactions.

sorry to bother again,

It is really user-friendly to click one time to send all transactions. I found ledger and math wallet in this multiple transactions case that they sign and send one transaction by one click operation and won't let the gas to cover all transactions which you could refer to if you are willing to.

max-mainnet avatar Nov 01 '22 19:11 max-mainnet

@max-mainnet I believe you are adding 300Tgas to each of the transactions you're sending to NETH.

That will not work 20 x 300 TGas is too much.

The limit for a single transaction is 300 TGas.

Maybe an explanation how NETH works: it takes all of the transactions you want to do and wraps them into a single NEAR Transaction.

This is signed by the Ethereum account and send to the smart contract on the NEAR Account.

The smart contract executes each transaction.

You can send the transaction to the contract with 300 Tgas, but ALL the transactions in your payload must not add up to more than 300 Tgas.

You didn't include your client side code, so it's hard to see what you did.

But I recommend something like:


wallet.signAndSendTransaction({
		receiverId: _contractId || contractId,
		actions: [
			{
				type: "FunctionCall",
				params: {
					methodName,
					args,
					gas: gas?.toString() || "30000000000000",
					deposit: attachedDeposit?.toString() || "0",
				},
			},
		],
	});

and you can easily create 20 TXs with 15 Tgas each action.

Hope it helps!

mattlockyer avatar Nov 01 '22 20:11 mattlockyer

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?

DamirSQA avatar Nov 02 '22 14:11 DamirSQA

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

Please see this explanation: https://github.com/near/wallet-selector/pull/520#issuecomment-1299067527

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.

mattlockyer avatar Nov 02 '22 19:11 mattlockyer

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?

DamirSQA avatar Nov 03 '22 12:11 DamirSQA

@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.

mattlockyer avatar Nov 03 '22 18:11 mattlockyer

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.

DamirSQA avatar Nov 04 '22 14:11 DamirSQA

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

mattlockyer avatar Nov 07 '22 14:11 mattlockyer