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

Anvil Custom Methods #766

Merged
merged 14 commits into from
Aug 23, 2023
Merged

Anvil Custom Methods #766

merged 14 commits into from
Aug 23, 2023

Conversation

Keyrxng
Copy link
Member

@Keyrxng Keyrxng commented Aug 20, 2023

Resolves #528

I have added a "Custom RPC Methods" button which expands to show the input area and CTA for the methods. I have added the methods mentioned but I'm curious whether or not I should just go ahead and add them all, or at least a few others maybe?

As for the data dumping call, should that hash just be written to a local file and pushed with everything else I assume?

Is there a need for some kind of frontend notification that the call was made? Anvil logs show them but nothing on the browser just now.

No testing done yet to confirm everything is working, thought I'd touch in and see if I'm on the right track first.

Screenshot 2023-08-20 114031
Screenshot 2023-08-20 114109
Screenshot 2023-08-20 120931

Handy anvil rpc methods. Viem set to replace Ethers anyway.
added anvil rpc methods to the sidebar
@0x4007
Copy link
Member

0x4007 commented Aug 20, 2023

curious whether or not I should just go ahead and add them all, or at least a few others maybe?

Your call. Use your best judgement! I didn't research this in a long time, but seems like a good idea to expose more tools to developers.

As for the data dumping call, should that hash just be written to a local file and pushed with everything else I assume?

I dont understand this question, perhaps @rndquu knows.

Is there a need for some kind of frontend notification that the call was made? Anvil logs show them but nothing on the browser just now.

Usually developers have their console open I assume thats sufficient? If you think its an issue, and its not in the specification, and its a substantial project, we can consider making a new issue for it.

@Keyrxng
Copy link
Member Author

Keyrxng commented Aug 20, 2023

No problem leave it with me.

I read through discussions about trying to persist the anvil server through a public domain and then talks about using the dataDump to perform the poor man's version of this for various scenarios so I wasn't sure if you had something in mind specifically for it yet.

Yeah I feel the same, it's pretty redundant to have a toast system set up for the messages if anvil is logging everything anyway.

Alright I'll keep on as I am for now then, cheers!

@rndquu
Copy link
Member

rndquu commented Aug 20, 2023

I have added the methods mentioned but I'm curious whether or not I should just go ahead and add them all, or at least a few others maybe?

Start with the ones mentioned in the issue. If you've added more then we appreciate it.

As for the data dumping call, should that hash just be written to a local file and pushed with everything else I assume?

As far as I understand anvil_dumpState returns a string representing the whole blockchain state. Can we simply download it as a file?

Is there a need for some kind of frontend notification that the call was made?

If anvil logs everything then there is no need to implement frontend notifications in the current issue.

90% of calls are working, have to look into the rest a bit more.
dumps, snapshots and anvil info can now be downloaded as a json file
@Keyrxng
Copy link
Member Author

Keyrxng commented Aug 20, 2023

I think that about wraps this up, unless there is anything else.

Any function can be removed by deleting the entry from method-configs.ts. Only Dumps, Anvil info and Snapshots are set for downloading at the moment. I've naively categorized them into three subsets of calls.

I'm going to look into the below points and will do what I can. It was my first and it was fun but I will become a more active contributor after such a good experience with it today. Let me know if there's anything further that needs looked at with this.

anvil_enableTraces makes a call but doesn't appear to do anything bar throw an error along the lines of "Not Implemented".

loggingEnabled also makes a call but doesn't appear to do anything.

MemPool functions require Geth integration.

So how does it work now, do I submit this ready for review or do I wait for correspondence first? For future reference I mean.

@0x4007
Copy link
Member

0x4007 commented Aug 21, 2023

If you believe the work is complete according to the specification, then you can convert this into a regular pull request and our review team will take a look as soon as possible. Thanks for your contribution!

@rndquu
Copy link
Member

rndquu commented Aug 21, 2023

@Keyrxng So this PR is ready for review, right?

@Keyrxng
Copy link
Member Author

Keyrxng commented Aug 21, 2023

Got one more push coming then I'll submit it for review, cheers!

removed unusable methods, added final methods, fixed download button.
@Keyrxng Keyrxng marked this pull request as ready for review August 21, 2023 12:06
@Keyrxng
Copy link
Member Author

Keyrxng commented Aug 21, 2023

The Build & Test check has failed during yarn install due to Lavamoat here but in my own attempts to have a deployed dapp instance I'm getting errors which are down to outdated deps.

Has there been a discussion about upgrading from Ethers to Viem?

Ethers vs Viem benchmarks.
Quick-read article.

Better performance. Smaller bundle size (~130kb to ~27kb). Rapid widespread adoption.

Wagmi should also be updated for a host of reasons, better-than-previous built-in react hooks as well as Wagmi/CLI integration as part of the build process would generate react hooks for any contract changes with input and output type checked ready to work straight out of the box.

I suggest this as I've used Viem when building this pr and from the looking around I've done regarding the build error it's more than likely the above is the problem.

Thoughts?

@0x4007
Copy link
Member

0x4007 commented Aug 21, 2023

Updating dependencies is fine but we will stick with Ethers because it's industry standard and that makes maintenance simpler. I've never heard of the alternative until now.

Copy link
Member

@rndquu rndquu left a comment

Choose a reason for hiding this comment

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

@Keyrxng

  1. https://github.com/ubiquity/ubiquity-dollar is a monorepo with 2 packages: contracts and dapp. This viem dependency is defined in the package.json root so it is assumed to be used by both packages but at the moment it is used only by the dapp package. Pls remove viem from the root's package.json and keep only this one.
  2. "Kebab case" workflow is failing. To fix it you could add the hardhat.config.js file to exceptions
  3. Pls fix the build workflow

Overall I don't understand why viem was chosen for RPC node communication while we use ethers.js everywhere in the project. Anyway I think we can keep it as a one-off solely for the "anvil cheats" feature.

    removed viem and just used next's fetch
@0x4007
Copy link
Member

0x4007 commented Aug 21, 2023

Why is a hardhat config in here? We made a special effort to remove hardhat as a dependency and only use Foundry. Again, this is for code maintainability reasons.

    fixed minor type errors
@ubiquibot
Copy link

ubiquibot bot commented Aug 22, 2023

@Keyrxng
Copy link
Member Author

Keyrxng commented Aug 22, 2023

Truthfully it was added because I saw that you were still using Hardhat as one of the default chains, so while I added the foundry chain import and commented out the Hardhat one as it clashes with both, I thought I should add the hardhat-foundry plugin thinking you guys were using tasks or something but thanks for clearing that up.

I've removed Viem altogether as it's not required to call the anvil methods. However, if you ever need to upgrade ConnectKit, WalletConnect or Wagmi you'll have to migrate. There is an open issue (unacknowledged) on the UniswapV3Core repo requesting Viem support, I expect long term they'll make the switch too but widgets still uses Ethers so all good.

I've fixed the check issues, cheers.

@0x4007
Copy link
Member

0x4007 commented Aug 22, 2023

What is the purpose of this "logo"? Can you fix or remove?

missing-logos


I tried a quick test, looks like the RPC methods log out in the console but unfortunately i couldn't verify if things work. I tried changing my balance but my balance would never update in the MetaMask UI (it would just remain on my mainnet balance!) I'm not sure whats the easiest way to test if things work.

using css to display download button
packages/dapp/package.json Outdated Show resolved Hide resolved
Co-authored-by: アレクサンダー.eth <[email protected]>
@0x4007
Copy link
Member

0x4007 commented Aug 22, 2023

  • Download button doesn't seem to do anything.
    • Edit: I see I need to "call" before hitting download. Perhaps you can adjust that in the UI so that it is intuitive? E.g. disable the download button until after call is pressed.
  • Also theres an error in rendering the table
client.js:1 Warning: validateDOMNesting(...): <tbody> cannot appear as a child of <div>.
    at tbody
    at div
    at div
    at div
    at div
    at AnvilRpcs (webpack-internal:///./components/layout/anvil/anvil-rpcs.tsx:18:82)
    at div
    at Sidebar
    at div
    at Layout (webpack-internal:///./components/layout/layout.tsx:67:11)
    at BalancesContextProvider (webpack-internal:///./components/lib/hooks/use-balances.tsx:26:11)
    at TransactionsContextProvider (webpack-internal:///./components/lib/hooks/use-transaction-logger.tsx:20:11)
    at Fe (webpack-internal:///../../node_modules/styled-components/dist/styled-components.browser.esm.js:31:17316)
    at ConnectKitProvider (webpack-internal:///../../node_modules/connectkit/build/index.es.js:9153:31)
    at QueryClientProvider (webpack-internal:///../../node_modules/@tanstack/react-query/build/lib/QueryClientProvider.mjs:48:3)
    at WagmiConfig (webpack-internal:///../../node_modules/wagmi/dist/index.js:134:3)
    at UseWeb3Provider (webpack-internal:///./components/lib/hooks/use-web-3.tsx:86:11)
    at CombinedComponents (webpack-internal:///./components/lib/combine-components.tsx:30:15)
    at CombinedComponents (webpack-internal:///./components/lib/combine-components.tsx:14:19)
    at CombinedComponents (webpack-internal:///./components/lib/combine-components.tsx:14:19)
    at CombinedComponents (webpack-internal:///./components/lib/combine-components.tsx:14:19)
    at Ubiquity (webpack-internal:///./pages/_app.tsx:37:11)
    at PathnameContextProviderAdapter (webpack-internal:///../../node_modules/next/dist/shared/lib/router/adapters.js:62:11)
    at ErrorBoundary (webpack-internal:///../../node_modules/next/dist/compiled/@next/react-dev-overlay/dist/client.js:303:63)
    at ReactDevOverlay (webpack-internal:///../../node_modules/next/dist/compiled/@next/react-dev-overlay/dist/client.js:852:919)
    at Container (webpack-internal:///../../node_modules/next/dist/client/index.js:62:1)
    at AppContainer (webpack-internal:///../../node_modules/next/dist/client/index.js:172:11)
    at Root (webpack-internal:///../../node_modules/next/dist/client/index.js:347:11)

appears once call is clicked
removed hardhat foundry plugin
@0x4007
Copy link
Member

0x4007 commented Aug 22, 2023

Any luck with the table rendering console error? Its not fatal, but still, shouldn't be there.

removed error logging
fixed failing calls, input errors from rpc call logged in result
@Keyrxng
Copy link
Member Author

Keyrxng commented Aug 22, 2023

I misread your comment at first @pavlovcik but methods should be fixed now.

You can also test things with cast but Metamask struggles to stay updated, the balance is reduce to 0 in the wallet but does update on chain.

image

@rndquu
Copy link
Member

rndquu commented Aug 22, 2023

@Keyrxng Is eth_sendTransaction implemented?

fixed send transaction
@Keyrxng
Copy link
Member Author

Keyrxng commented Aug 23, 2023

Okay so I have managed to get eth_sendTransaction working but there's a caveat:

We can only send transactions from either the 10 built-in accounts or one we are have the private key for, we can't sign transactions from impersonated accounts. Sending from one of the built-in accounts automatically signs the transaction under the hood.

We have eth_sendRawTransaction as well but quite a bit of setup to get this fully operational I think, it's callable and executes but the transaction object would need to be built properly first.

Also, Eth Sign only works when called from the built-in accounts it seems.

I also noticed that the 10 built-in accounts served using yarn start:anvil are different from those that are served with the default standalone anvil instance due to the mnemonic being passed to anvil.ts generating a different set of 10.

@Keyrxng
Copy link
Member Author

Keyrxng commented Aug 23, 2023

There are a few that would need quite a bit more work in order to get set up but could be very beneficial like the set, get & remove block filter functions as well as the get & filter logs.

The three Txpool functions would require a GETH integration so they have been removed for now.

loggingEnabled and enableTraces definitely aren't fully functional so they've been removed for now as well.

As a lot of the optional inputs have been left out (particularly sendTransaction) things could be improved a lot more to allow these to be modified as well, if that was the case it would be good to have a clear list of which functions to expand on specifically (filters, logs etc) perhaps as another issue as the spec here has been achieved I think.

@0x4007
Copy link
Member

0x4007 commented Aug 23, 2023

We have eth_sendRawTransaction as well but quite a bit of setup to get this fully operational I think, it's callable and executes but the transaction object would need to be built properly first.

There are a few that would need quite a bit more work in order to get set up but could be very beneficial like the set, get & remove block filter functions as well as the get & filter logs.

We can make new tasks for these if they are projects, and if you can help us spec it out. @rndquu if you know what is a priority please feel free to make those tasks and get them funded as well.

@rndquu
Copy link
Member

rndquu commented Aug 23, 2023

@Keyrxng awesome, thank you

@rndquu rndquu merged commit 8fa34df into ubiquity:development Aug 23, 2023
9 checks passed
@Keyrxng
Copy link
Member Author

Keyrxng commented Aug 23, 2023

The first of many (¬‿¬)

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.

Create UI for custom RPC methods
3 participants