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

Upgrade to latest wagmi version #350

Merged
merged 5 commits into from
Aug 30, 2023
Merged

Upgrade to latest wagmi version #350

merged 5 commits into from
Aug 30, 2023

Conversation

neokry
Copy link
Collaborator

@neokry neokry commented Aug 16, 2023

Description

  • upgrades to wagmi v1

Code review

  • does the app still function properly specifically around contract data fetching, writing and listening for events.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • I have done a self-review of my own code
  • Any new and existing tests pass locally with my changes
  • My changes generate no new warnings (lint warnings, console warnings, etc)

@vercel
Copy link

vercel bot commented Aug 16, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
nouns-builder ✅ Ready (Inspect) Visit Preview Aug 28, 2023 5:09am
testnet-nouns-builder ✅ Ready (Inspect) Visit Preview Aug 28, 2023 5:09am

@neokry neokry mentioned this pull request Aug 18, 2023
6 tasks
import { SDK } from '../client'
import { AuctionBidFragment } from '../sdk.generated'

export const getBids = async (chainId: CHAIN_ID, collection: string, tokenId: string) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps some error handling?

@@ -59,7 +45,7 @@ export function LayoutWrapper({ children }: { children: ReactNode }) {
}

//@ts-ignore
if (isBlocked(signer?._address))
if (isBlocked(address))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we still need that TS ignore?

Looks like the cause of the problem before was that _address wasn't found on type signer.

@@ -43,7 +42,7 @@ export const useAuctionEvents = ({
)

await mutate([SWR_KEYS.AUCTION_BIDS, chainId, auction, tokenId], () =>
getBids(chainId, auction as string, tokenId)
getBids(chainId, collection, tokenId!.toString())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Curious why you have to use the '!' assertions.

Copy link
Collaborator Author

@neokry neokry Aug 28, 2023

Choose a reason for hiding this comment

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

switched to a cast instead

@@ -125,7 +119,7 @@ export const AllocationForm: React.FC<AllocationFormProps> = ({ title }) => {
setActiveSection(activeSection + 1)
}

if (!signerAddress) return null
if (!address) return null
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the rare case that address is null, do we really want the component to return null?

Copy link
Collaborator Author

@neokry neokry Aug 28, 2023

Choose a reason for hiding this comment

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

this should be fixed in a separate pr or when we rework the create dao flow. keeping as is for now to minimize changes in this upgrade

)

if (isLoading || thresholdIsLoading) return null

const tokensNeeded = proposalThreshold && proposalThreshold.toNumber() + 1
const tokensNeeded =
proposalThreshold !== undefined ? Number(proposalThreshold) + 1 : undefined
Copy link
Collaborator

Choose a reason for hiding this comment

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

Curious, is this proposalThreshold !== undefined stuff because of BigInt (ex. 0n)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes 0 is a valid proposal threshold so the proposalThreshold && check didn't work when moving from BigNunber to BigInt

const submitCallback = React.useCallback(
(values: { transactionCustomABI: string }) => {
try {
if (!!signer && !!customTransaction.address && !!values.transactionCustomABI) {
if (!!customTransaction.address && !!values.transactionCustomABI) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why the double bang truthy tests 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.

removing this not sure why they are written like this either

imageUri,
]
) || '',
calldata: encodeFunctionData({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wow, so much cleaner.

@@ -68,20 +72,20 @@ const VoteModal: React.FC<{
const config = await prepareWriteContract({
...governorContractParams,
functionName: 'castVoteWithReason',
args: [proposalId as BytesType, BigNumber.from(values.choice), values.reason],
args: [proposalId as BytesType, BigInt(values.choice!), values.reason],
Copy link
Collaborator

Choose a reason for hiding this comment

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

would opt for the type assertion over the bang assertion unless necessary.

@neokry neokry merged commit 617d6de into main Aug 30, 2023
2 of 3 checks passed
@neokry neokry deleted the wagmi-upgrade branch August 30, 2023 06:50
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

2 participants