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: Add view for get/set ERC20 balances (#39) #50

Merged
merged 13 commits into from
Sep 13, 2023
Merged

Conversation

ditto-eth
Copy link
Contributor

@ditto-eth ditto-eth commented Sep 6, 2023

vid: https://twitter.com/dittoproj/status/1699904944898240966?s=20

  • button to go to account view
  • add tokens
  • view bal
  • remove tokens
  • set bal
    • handle tokens that change balanceOf like rebasing token (stETH)
    • handle tokens that have different decimals (udsc) using formatUnits/parseUnits
  • not doing erc721/1115 in this pr
  • caveat: not handling tokens that use special slots for balances (mentioned above)
  • not changing totalSupply, but can do it using the same approach as this
  • updates balances immediately upon tx (a swap and it changes the balances page without reload) - doesn't do this the other way around without doing a mine/next block

@jxom said we can add a new issue to embed revm to do this better but my workaround might work for now for most tokens.

This setERCBalance is a bit like fuzzing! By assuming the balanceOf mapping is usually near slot 0, we can iterate through each and randomly check values to set we are setting the right storage slot.

  • loop through each slot, starting from 0 (maybe stop at some point, I just hardcoded it to 10)
  • setStorageAt() with a special value (if we used the user value, it might be 0 or a number that a slot might already be so I want to make sure it's weird enough that it's obvious we calculated it right I think?)
  • check the val of balanceOf == value
  • if not, guess++ (get the next slot, from 0 to 1, etc)
  • if yes, profit! (we've found it!, maybe this could be saved in the tokens store locally, or could just hardcode special tokens)

Note: one issue is ERC20 tokens that use special slots (assembly, diamond storage, etc) - this stops checking at slot 10. Those would have to be hardcoded or we could read the source code to find the storage constant, not sure how easy that is?

account-select
token-view

q:

  • should erc20 be stored separately from 721/1155? Or put all into one as diff types
  • copied the abi file since not importing from wagmi

@ditto-eth ditto-eth changed the title feat: Add view for viewing ERC20 (#39) feat: Add view for get/set ERC20 balances (#39) Sep 7, 2023
@gakonst
Copy link
Member

gakonst commented Sep 8, 2023

Note: one issue is ERC20 tokens that use special slots (assembly, diamond storage, etc) - this stops checking at slot 10. Those would have to be hardcoded or we could read the source code to find the storage constant, not sure how easy that is?

I think we can merge w/o this, document as a known limitation, and we can add an issue for future usage. Running with the revm in WASM & capturing the storage slots accessed during balanceOf is useful here. I'll see if I can get us a nice library for this, similar to https://github.com/gakonst/pyrevm

contract.read.name(),
contract.read.symbol(),
contract.read.decimals(),
contract.read.balanceOf([address]),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe for next pr: it's probably better to just leave this as only the balanceOf call, and then add the name/symbol/decimals metadata to the tokens store?

It should attempt to validate that the address is an erc20 (not sure how really), and maybe include if it's 721, which can save the tokenUri if it exists? q is whether we want multiple stores for 20/721/1155 or if it's possible to keep in 1 and just filter it.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I'd consider splitting this up into useErc20Balance and useErc20Metadata Hooks, that both use React Query. Two reasons:

  1. Metadata is independent of address, and we can cache that separately (instead of having it as separate cache entries for erc/address pairs).
  2. We can still just use React Query still for useErc20Metadata as syncing it into Zustand probably isn't worth it (syncing RQ with external stores like Zustand is generally not recommended).
  3. Invalidating metadata is useless since it's immutable (https://github.com/paradigmxyz/rivet/pull/50/files#diff-83dac5097fa3ff04758ab51b989502a815d5ef851830cd4f9aee80856305c6f9R59-R61)

Might be worth doing in this PR as it's pretty trivial.

src/utils/abi.ts Outdated Show resolved Hide resolved
contract.read.name(),
contract.read.symbol(),
contract.read.decimals(),
contract.read.balanceOf([address]),
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I'd consider splitting this up into useErc20Balance and useErc20Metadata Hooks, that both use React Query. Two reasons:

  1. Metadata is independent of address, and we can cache that separately (instead of having it as separate cache entries for erc/address pairs).
  2. We can still just use React Query still for useErc20Metadata as syncing it into Zustand probably isn't worth it (syncing RQ with external stores like Zustand is generally not recommended).
  3. Invalidating metadata is useless since it's immutable (https://github.com/paradigmxyz/rivet/pull/50/files#diff-83dac5097fa3ff04758ab51b989502a815d5ef851830cd4f9aee80856305c6f9R59-R61)

Might be worth doing in this PR as it's pretty trivial.

Copy link
Member

Choose a reason for hiding this comment

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

rename to useErc20Balance?


type UseErcBalanceParameters = {
address: Address
erc: Address
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
erc: Address
contractAddress: Address

Comment on lines 20 to 21
erc,
address,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
erc,
address,
address,
tokenAddress,

queryKey: getErcBalanceQueryKey([client.key, { erc, address }]),
async queryFn() {
const contract = getContract({
address: erc,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
address: erc,
address: tokenAddress,

Comment on lines 126 to 128
if (slotGuess >= 10n) {
throw "couldn't find balancesOf mapping past slot 10"
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (slotGuess >= 10n) {
throw "couldn't find balancesOf mapping past slot 10"
}
if (slotGuess >= 10n)
throw new BaseError('could not find storage for: `balanceOf`')

}
}
} catch (e) {
console.error(e)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this try/catch? Can we remove and let React Query catch the error?

@@ -251,6 +254,34 @@ function RemoveButton({ onClick }: { onClick: (e: any) => void }) {
)
}

export function DetailButton({ onClick }: { onClick: (e: any) => void }) {
Copy link
Member

Choose a reason for hiding this comment

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

FYI - we have a Button.Symbol now!

Comment on lines 3 to 12
type SetErcBalanceParameters = {
address: Address
erc: Address
value: bigint
}

import { type Address, encodeAbiParameters, keccak256, pad, toHex } from 'viem'
import { queryClient } from '~/react-query'
import { useClient } from './useClient'
import { getErcBalanceQueryKey } from './useErcBalance'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
type SetErcBalanceParameters = {
address: Address
erc: Address
value: bigint
}
import { type Address, encodeAbiParameters, keccak256, pad, toHex } from 'viem'
import { queryClient } from '~/react-query'
import { useClient } from './useClient'
import { getErcBalanceQueryKey } from './useErcBalance'
import { type Address, encodeAbiParameters, keccak256, pad, toHex } from 'viem'
import { queryClient } from '~/react-query'
import { useClient } from './useClient'
import { getErcBalanceQueryKey } from './useErcBalance'
type SetErcBalanceParameters = {
address: Address
erc: Address
value: bigint
}

src/utils/abi.ts Outdated Show resolved Hide resolved
Comment on lines 5 to 7
export type TokensState = {
tokens: Address[]
}
Copy link
Member

@jxom jxom Sep 12, 2023

Choose a reason for hiding this comment

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

We will need to make sure this works across accounts too.

Suggested change
export type TokensState = {
tokens: Address[]
}
export type TokensState = {
tokens: Record<Address, Address[]>
}

@jxom jxom merged commit a184297 into paradigmxyz:main Sep 13, 2023
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.

3 participants