Remove azorius typechain #1671

adamgall · 2024-05-07T23:09:17Z

Please review and merge #1670 first.

netlify · 2024-05-07T23:09:34Z

✅ Deploy Preview for fractal-dev ready!

Name	Link
🔨 Latest commit	`bcc20e3`
🔍 Latest deploy log	https://app.netlify.com/sites/fractal-dev/deploys/664a64a362526c00087ab046
😎 Deploy Preview	https://deploy-preview-1671.app.dev.fractalframework.xyz
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Da-Colon

🔥

mudrila

Lots of comments, but should be good to go afterwards

mudrila · 2024-05-23T11:52:18Z

src/components/ui/proposal/useProposalCountdown.tsx

@@ -62,7 +62,7 @@ export function useProposalCountdown(proposal: FractalProposal) {
        (async () => {
          try {
            if (dao?.isAzorius) {
-              await updateProposalState(BigInt(proposal.proposalId));
+              await updateProposalState(Number(proposal.proposalId));


Do we even need casting here? Why can't we just pass number?

proposal.proposalId is a string type here

mudrila · 2024-05-23T11:53:25Z

src/hooks/DAO/loaders/governance/useAzoriusListeners.ts

+            log.args.proposer,
+            azoriusContract,
+            provider,
+            Promise.resolve(undefined),


I don't really like this. Can we adjust mapper so that it's not expecting always promise?

Maybe we should adjust mapper to expect array instead

src/hooks/DAO/loaders/governance/useAzoriusListeners.ts

mudrila · 2024-05-23T13:05:31Z

src/hooks/DAO/loaders/governance/useAzoriusProposals.ts

-          proposalCreatedEvent.args.proposalId.toBigInt(),
-          proposalCreatedEvent.args.proposer,
+          Number(proposalCreatedEvent.args.proposalId),
+          proposalCreatedEvent.args.proposer || '',


This feels wrong - proposer can't be empty string I think as it comes from message sender

Agreed. For some reason which I haven't figured out yet, the viem arg properties on events all are optionally undefined, even if the solidity ABI indicates that they will definitely be there. Regardless, I've updated the code here to just early exity if (for some very unlikely reason in practice) the proposer is undefined.

src/hooks/DAO/loaders/governance/useAzoriusProposals.ts

mudrila · 2024-05-23T13:53:11Z

src/hooks/utils/useCanUserSubmitProposal.ts

          // @dev assumes the first strategy is the voting contract
          const votingContractAddress = (
-            await azoriusContract.getStrategies(SENTINEL_ADDRESS, 0)
+            await azoriusContract.read.getStrategies([SENTINEL_ADDRESS, 0n])


mudrila · 2024-05-23T13:53:39Z

src/hooks/utils/useMasterCopy.ts

@@ -16,6 +16,7 @@ export function useMasterCopy() {
      linearVotingMasterCopy,
      linearVotingERC721MasterCopy,
      fractalModuleMasterCopy,
+      fractalAzoriusMasterCopy,


Rename to azoriusMasterCopy pls

i will later sir not here though

mudrila · 2024-05-23T13:53:55Z

src/hooks/utils/useMasterCopy.ts

-      masterCopyAddress === baseContracts?.fractalAzoriusMasterCopyContract.asProvider.address,
-    [baseContracts],
+    (masterCopyAddress: Address) => masterCopyAddress === fractalAzoriusMasterCopy,
+    [fractalAzoriusMasterCopy],


i will but not in this PR sir

mudrila · 2024-05-23T13:54:47Z

src/models/AzoriusTxBuilder.ts

-      this.predictedAzoriusAddress!,
-      this.signerOrProvider,
-    );
+    this.azoriusAddress = this.predictedAzoriusAddress;


Can't we just use this.predictedAzoriusAddress instead of adding this.azoriusAddress field?

yes we can!

well actually, i don't know if that's safe to do. this.azoriusAddress comes from some inherited class, it might be being used in other files. i'm going to leave as is until i get a chance to refactor this whole transaction builder process, if that ever becomes a priority.

mudrila · 2024-05-23T13:55:54Z

src/types/fractal.ts

-  moduleContract:
-    | Azorius
-    | GetContractReturnType<typeof FractalModuleAbi, PublicClient>
-    | undefined;
  moduleAddress: string;


Guess we can simplify those fields to just address and type, but not a big deal to keep them as is

you mean the field names?

rename moduleAddress to address
rename moduleType to type
?

mudrila

Bunch of more changes requested, but we're almost there sir

mudrila · 2024-05-24T14:08:44Z

src/hooks/DAO/loaders/useGovernanceContracts.ts

+      await azoriusContract.read.getStrategies([SENTINEL_ADDRESS, 0n])
+    )[1];
+
+    const masterCopyData = await getZodiacModuleProxyMasterCopyData(


So instead of doing

const masterCopyData = await getZodiacModuleProxyMasterCopyData( getAddress(votingStrategyAddress), ); const isOzLinearVoting = masterCopyData.isOzLinearVoting; const isOzLinearVotingERC721 = masterCopyData.isOzLinearVotingERC721;

We can do

const { isOzLinearVoting, isOzLinearVotingERC721 } = await getZodiacModuleProxyMasterCopyData( getAddress(votingStrategyAddress), );

mudrila · 2024-05-24T14:09:19Z

src/hooks/DAO/loaders/useGovernanceContracts.ts

@@ -20,105 +19,106 @@ export const useGovernanceContracts = () => {
  const { getZodiacModuleProxyMasterCopyData } = useMasterCopy();
  const publicClient = usePublicClient();

+  const votingStrategyAddress = useVotingStrategyAddress();


That's what I'm talking about, so much red
Let it burn baby 🔥

This voting strategy address should really be part of state though, huh

src/hooks/DAO/proposal/useSubmitProposal.ts

mudrila · 2024-05-24T14:11:26Z

src/hooks/DAO/proposal/useSubmitProposal.ts

  const { data: walletClient } = useWalletClient();
+  const publicClient = usePublicClient();
+
+  const votingContractAddress = useVotingStrategyAddress();


Optional - you can rename this to votingStrategyAddress and then use prop shorthand. Also hook is called useVotingStrategyAddress so I'd expect rather votingStrategyAddress variable name

The reason I left this as votingContractAddress is because votingStrategyAddress is already being used as a variable name elsewhere in this file, and I get yelled at about same name on different scopes. I'll change this to votingStrategyContractAddress to meet in the middle.

mudrila · 2024-05-24T14:12:24Z

src/hooks/DAO/proposal/useSubmitProposal.ts

-            azoriusContract: azoriusModule.moduleContract as Azorius,
-            votingStrategyAddress,
+            azoriusAddress: getAddress(azoriusModule.moduleAddress),
+            votingStrategyAddress: votingContractAddress,


So if you'd rename votingContractAddress - you will be able to use prop shorthand and instead of votingStrategyAddress: votingContractAddress, it will be just votingStrategyAddress,

mudrila · 2024-05-24T14:12:52Z

src/hooks/DAO/proposal/useUserERC721VotingTokens.ts

  const safeAPI = useSafeAPI();
  const publicClient = usePublicClient();

+  const votingContractAddress = useVotingStrategyAddress();


Again - why naming it votingContractAddress and not votingStrategyAddress? Feel like that's a bit confusing

For this one, it was a clean rename 👍

mudrila · 2024-05-24T14:14:49Z

src/hooks/utils/useCanUserSubmitProposal.ts

-        const safeModules = await lookupModules(safeInfo.modules);
-        const azoriusModule = getAzoriusModuleFromModules(safeModules);
-
-        if (azoriusModule && azoriusModule.moduleContract) {


I think here we still wanna figure this out "on fly" so we probably will need to transform that hook so that we can get votingContractAddress for any safe by address, and not just the one from "global context"

This logic is used to figure out whether you can create subSafe, execute clawback and child safes from the "Organization" page, as far as I remember it right 😄

Perfect, exactly the kind of feedback I was hoping for. Thanks for your suggestions on how to clean up the hook I created to support this usecase

mudrila · 2024-05-24T14:20:55Z

src/hooks/utils/useVotingStrategyAddress.ts

+  const { node } = useFractal();
+  const publicClient = usePublicClient();
+
+  useEffect(() => {


Instead of using useEffect and useState - I'd suggest transforming this into a function, that will be called "on demand" - this will preserve existing functionality
So then it will be something like

const useVotingStrategyAddress = () => { const getVotingStrategyAddress = useCallback((safeAddress?: Address) => { ... blah blah all the logic of getting strategy address depending on whether safe address is provided. If it is - we're getting data for specific safe using safeAPI, otherwise - we're falling back to global context blah blah ... }, [publicClient, node, whateverElseGoesInDependencies] ); return { getVotingStrategyAddress } }

Most crucial part here is to have this helper function, but you can keep useEffect and useState as well if you wanna avoid calling getVotingStrategyAddress when it's not necessary

Ok ok i got u

adamgall · 2024-05-24T17:29:38Z

@mudrila addressed your changes, plz review again!

…lContracts

…safe address

netlify · 2024-05-29T13:04:43Z

✅ Deploy Preview for decent-interface-dev ready!

Name	Link
🔨 Latest commit	`45045dd`
🔍 Latest deploy log	https://app.netlify.com/sites/decent-interface-dev/deploys/665737ec5f84a600082f23ae
😎 Deploy Preview	https://deploy-preview-1671.app.dev.decentdao.org
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

mudrila

I'm seeing 2 linting warnings about zeroAddress is defined but never used
Apart of this and 2 other minor comments - looks good to go, so leaving my approval

mudrila · 2024-05-29T14:03:04Z

src/hooks/utils/useCanUserSubmitProposal.ts

@@ -89,7 +79,7 @@ export function useCanUserCreateProposal() {
    },
    [
      erc721LinearVotingContractAddress,
-      lookupModules,
+      getVotingStrategyAddress,


We're putting it into dependencies array but not wrapping the function itself into useCallback - this might cause performance issues, barely noticeable I think but might lead to minor problems on "Organizations" page. I'd suggest wrapping getVotingStrategyAddress into useCallback

adamgall · 2024-05-29T14:14:28Z

@mudrila fixed both of your latest comments and pushed

adamgall requested review from DarksightKellar, mudrila and Da-Colon May 7, 2024 23:09

adamgall self-assigned this May 7, 2024

adamgall force-pushed the remove-fractalModule-typechain branch from abd45a7 to 81f64b9 Compare May 7, 2024 23:29

adamgall force-pushed the remove-azorius-typechain branch from 40f67e7 to 8806167 Compare May 7, 2024 23:29

Da-Colon approved these changes May 7, 2024

View reviewed changes

adamgall force-pushed the remove-fractalModule-typechain branch from 81f64b9 to f8f3761 Compare May 8, 2024 00:10

adamgall force-pushed the remove-azorius-typechain branch from 8806167 to 77fbc09 Compare May 8, 2024 00:10

adamgall force-pushed the remove-fractalModule-typechain branch from f8f3761 to 558837e Compare May 8, 2024 00:14

adamgall force-pushed the remove-azorius-typechain branch from 77fbc09 to 05f97e2 Compare May 8, 2024 00:14

adamgall force-pushed the remove-fractalModule-typechain branch from 558837e to 61cf1c2 Compare May 8, 2024 20:13

adamgall force-pushed the remove-azorius-typechain branch from 05f97e2 to 175f1ef Compare May 8, 2024 20:14

adamgall force-pushed the remove-fractalModule-typechain branch from 61cf1c2 to 713fa33 Compare May 14, 2024 14:00

adamgall force-pushed the remove-azorius-typechain branch from 175f1ef to 25dcfd5 Compare May 14, 2024 14:02

adamgall force-pushed the remove-fractalModule-typechain branch from 713fa33 to 4196474 Compare May 15, 2024 15:27

adamgall force-pushed the remove-azorius-typechain branch from 25dcfd5 to 88d1a1f Compare May 15, 2024 15:28

adamgall force-pushed the remove-fractalModule-typechain branch from 4196474 to e292d84 Compare May 15, 2024 16:41

adamgall force-pushed the remove-azorius-typechain branch from 88d1a1f to 44286f6 Compare May 15, 2024 16:41

adamgall force-pushed the remove-fractalModule-typechain branch from e292d84 to f93caf5 Compare May 19, 2024 20:44

adamgall force-pushed the remove-azorius-typechain branch from 44286f6 to bcc20e3 Compare May 19, 2024 20:44

adamgall force-pushed the remove-fractalModule-typechain branch from f93caf5 to 8d04e30 Compare May 23, 2024 12:03

adamgall force-pushed the remove-azorius-typechain branch from bcc20e3 to 97594c2 Compare May 23, 2024 12:03

adamgall force-pushed the remove-fractalModule-typechain branch from 8d04e30 to 6810626 Compare May 23, 2024 13:01

adamgall force-pushed the remove-azorius-typechain branch from 97594c2 to 8a5e7f1 Compare May 23, 2024 13:02

adamgall changed the base branch from remove-fractalModule-typechain to root/remove-typechain May 23, 2024 13:24

adamgall marked this pull request as ready for review May 23, 2024 13:24

adamgall force-pushed the remove-azorius-typechain branch from 8a5e7f1 to 00fd68f Compare May 23, 2024 13:25

mudrila requested changes May 23, 2024

View reviewed changes

adamgall force-pushed the remove-azorius-typechain branch from 00fd68f to f99de9c Compare May 23, 2024 14:06

mudrila requested changes May 24, 2024

View reviewed changes

adamgall force-pushed the remove-azorius-typechain branch from 7fd770c to 984dc15 Compare May 24, 2024 17:52

adamgall added 10 commits May 29, 2024 09:03

Add AzoriusAbi

b2c889b

Remove fractalAzoriusMasterCopyContract from AzoriusContracts, Fracta…

a3cf0ad

…lContracts

Remove more instances of Azorius typechain

a9455de

Remove more instances of Azorius typechain

59c681d

Early exit if some event args are undefined

d31a824

No longer passing unresolved promises around

535b801

Rename an action

3a5b859

Remove unnecessary optional check

e0da54f

DRY "finding" strategy address into a hook

55f4dea

useVotingStrategyAddress now returns function which accepts optional …

db4b339

…safe address

adamgall force-pushed the remove-azorius-typechain branch from 984dc15 to db4b339 Compare May 29, 2024 13:03

mudrila approved these changes May 29, 2024

View reviewed changes

adamgall added 2 commits May 29, 2024 10:06

Remove unused imports

47aeefe

Wrap getVotingStrategyAddress in a useCallback

45045dd

adamgall merged commit 7b2f359 into root/remove-typechain May 29, 2024
7 checks passed

adamgall deleted the remove-azorius-typechain branch May 29, 2024 15:08

Remove azorius typechain #1671

Remove azorius typechain #1671

Conversation

adamgall commented May 7, 2024

netlify bot commented May 7, 2024 • edited Loading

✅ Deploy Preview for fractal-dev ready!

Da-Colon left a comment

Choose a reason for hiding this comment

mudrila left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mudrila left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamgall commented May 24, 2024

netlify bot commented May 29, 2024 • edited Loading

✅ Deploy Preview for decent-interface-dev ready!

mudrila left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamgall commented May 29, 2024

netlify bot commented May 7, 2024 •

edited

Loading

netlify bot commented May 29, 2024 •

edited

Loading