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.

Sign up for GitHub

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

Remove VotesERC20 from AzoriusTxBuilder #1645

Merged
merged 4 commits into from
May 7, 2024
Merged

Remove VotesERC20 from AzoriusTxBuilder #1645

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9d402b3
Create second flavor of "buildContractCall" to do it viem-style
adamgall May 6, 2024
1f0e490
Remove VotesERC20 imports from AzoriusTxBuilder, deal with all downst…
adamgall May 6, 2024
3863404
Don't need to create a contract object, address will do
adamgall May 7, 2024
b72f908
Lint
adamgall May 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 46 additions & 7 deletions src/helpers/crypto.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ import {
bytesToBigInt,
Hex,
isHex,
encodeFunctionData,
Abi,
} from 'viem';
import { ContractConnection } from '../types';
import { MetaTransaction, SafePostTransaction, SafeTransaction } from '../types/transaction';
Expand Down Expand Up @@ -161,20 +163,18 @@ export const buildSafeAPIPost = async (
};
};

export const buildContractCall = (
contract: Contract,
method: string,
params: any[],
const finishBuildingConractCall = (
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: finishBuildingConractCall -> finishBuildingContractCall

Copy link
Member Author

Choose a reason for hiding this comment

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

gah. i almost just want to leave it as is for now, because fixing this will cause me to need to rebase all of the branches above this one 😅

i plan on consolidating this back into one function anyway, once the the ethers-style function can be removed. I'll deal with it then.

data: Hex,
nonce: number,
contractAddress: Address,
delegateCall?: boolean,
overrides?: Partial<SafeTransaction>,
): SafeTransaction => {
const data = contract.interface.encodeFunctionData(method, params);
) => {
const operation: 0 | 1 = delegateCall ? 1 : 0;
return buildSafeTransaction(
Object.assign(
{
to: contract.address,
to: contractAddress,
data,
operation,
nonce,
Expand All @@ -184,6 +184,45 @@ export const buildContractCall = (
);
};

export const buildContractCall = (
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this meant to go away at some point? Noticing the -viem variant below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure is!

contract: Contract,
method: string,
params: any[],
nonce: number,
delegateCall?: boolean,
overrides?: Partial<SafeTransaction>,
): SafeTransaction => {
const data = contract.interface.encodeFunctionData(method, params);
if (!isHex(data)) {
throw new Error('encoded function data not hexadecimal');
}
return finishBuildingConractCall(
data,
nonce,
getAddress(contract.address),
delegateCall,
overrides,
);
};

export const buildContractCallViem = (
contractAbi: Abi,
contractAddress: Address,
method: string,
params: any[],
nonce: number,
delegateCall?: boolean,
overrides?: Partial<SafeTransaction>,
): SafeTransaction => {
const data = encodeFunctionData({
abi: contractAbi,
functionName: method,
args: params,
});

return finishBuildingConractCall(data, nonce, contractAddress, delegateCall, overrides);
};

const encodeMetaTransaction = (tx: MetaTransaction): string => {
const txDataBytes = toBytes(tx.data);
const txDataHex = toHex(txDataBytes);
Expand Down
6 changes: 5 additions & 1 deletion src/hooks/DAO/useBuildDAOTx.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { useCallback } from 'react';
import { usePublicClient } from 'wagmi';
import { TxBuilderFactory } from '../../models/TxBuilderFactory';
import { useFractal } from '../../providers/App/AppProvider';
import { useNetworkConfig } from '../../providers/NetworkConfig/NetworkConfigProvider';
Expand Down Expand Up @@ -27,6 +28,7 @@ const useBuildDAOTx = () => {
governance,
governanceContracts: { erc721LinearVotingContractAddress },
} = useFractal();
const publicClient = usePublicClient();

const buildDao = useCallback(
async (
Expand All @@ -36,7 +38,7 @@ const useBuildDAOTx = () => {
) => {
let azoriusContracts: AzoriusContracts | undefined;

if (!user.address || !signerOrProvider || !baseContracts) {
if (!user.address || !signerOrProvider || !baseContracts || !publicClient) {
return;
}
const {
Expand Down Expand Up @@ -102,6 +104,7 @@ const useBuildDAOTx = () => {

const txBuilderFactory = new TxBuilderFactory(
signerOrProvider,
publicClient,
buildrerBaseContracts,
azoriusContracts,
daoData,
Expand Down Expand Up @@ -148,6 +151,7 @@ const useBuildDAOTx = () => {
[
user.address,
signerOrProvider,
publicClient,
baseContracts,
erc721LinearVotingContractAddress,
dao,
Expand Down
7 changes: 6 additions & 1 deletion src/hooks/DAO/useDeployAzorius.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { useCallback } from 'react';
import { useTranslation } from 'react-i18next';
import { useNavigate } from 'react-router-dom';
import { getAddress, isHex } from 'viem';
import { usePublicClient } from 'wagmi';
import { DAO_ROUTES } from '../../constants/routes';
import { TxBuilderFactory } from '../../models/TxBuilderFactory';
import { useFractal } from '../../providers/App/AppProvider';
Expand Down Expand Up @@ -32,13 +33,15 @@ const useDeployAzorius = () => {
const { t } = useTranslation(['transaction', 'proposalMetadata']);
const { submitProposal } = useSubmitProposal();
const { canUserCreateProposal } = useCanUserCreateProposal();
const publicClient = usePublicClient();

const deployAzorius = useCallback(
async (
daoData: AzoriusERC20DAO | AzoriusERC721DAO,
shouldSetName?: boolean,
shouldSetSnapshot?: boolean,
) => {
if (!daoAddress || !canUserCreateProposal || !safe || !baseContracts) {
if (!daoAddress || !canUserCreateProposal || !safe || !baseContracts || !publicClient) {
return;
}
const {
Expand Down Expand Up @@ -81,6 +84,7 @@ const useDeployAzorius = () => {

const txBuilderFactory = new TxBuilderFactory(
signerOrProvider,
publicClient,
builderBaseContracts,
azoriusContracts,
daoData,
Expand Down Expand Up @@ -135,6 +139,7 @@ const useDeployAzorius = () => {
},
[
signerOrProvider,
publicClient,
baseContracts,
t,
canUserCreateProposal,
Expand Down
27 changes: 17 additions & 10 deletions src/models/AzoriusTxBuilder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@ import {
LinearERC20Voting__factory,
LinearERC721Voting,
LinearERC721Voting__factory,
VotesERC20,
VotesERC20__factory,
} from '@fractal-framework/fractal-contracts';
import {
getCreate2Address,
Expand All @@ -20,12 +18,13 @@ import {
isAddress,
isHex,
encodeFunctionData,
PublicClient,
} from 'viem';
import VotesERC20Abi from '../assets/abi/VotesERC20';
import VotesERC20WrapperAbi from '../assets/abi/VotesERC20Wrapper';
import { GnosisSafeL2 } from '../assets/typechain-types/usul/@gnosis.pm/safe-contracts/contracts';
import { SENTINEL_ADDRESS } from '../constants/common';
import { buildContractCall, getRandomBytes } from '../helpers';
import { buildContractCall, buildContractCallViem, getRandomBytes } from '../helpers';
import {
BaseContracts,
SafeTransaction,
Expand Down Expand Up @@ -55,7 +54,7 @@ export class AzoriusTxBuilder extends BaseTxBuilder {
public azoriusContract: Azorius | undefined;
public linearVotingContract: LinearERC20Voting | undefined;
public linearERC721VotingContract: LinearERC721Voting | undefined;
public votesTokenContract: VotesERC20 | undefined;
public votesTokenContractAddress: Address | undefined;

private votesERC20WrapperMasterCopyAddress: string;
private votesERC20MasterCopyAddress: string;
Expand All @@ -67,6 +66,7 @@ export class AzoriusTxBuilder extends BaseTxBuilder {

constructor(
signerOrProvider: any,
publicClient: PublicClient,
baseContracts: BaseContracts,
azoriusContracts: AzoriusContracts,
daoData: AzoriusERC20DAO | AzoriusERC721DAO,
Expand All @@ -78,6 +78,7 @@ export class AzoriusTxBuilder extends BaseTxBuilder {
) {
super(
signerOrProvider,
publicClient,
baseContracts,
azoriusContracts,
daoData,
Expand Down Expand Up @@ -245,9 +246,14 @@ export class AzoriusTxBuilder extends BaseTxBuilder {
}

public buildApproveClaimAllocation() {
if (!this.votesTokenContractAddress) {
return;
}

const azoriusGovernanceDaoData = this.daoData as AzoriusERC20DAO;
return buildContractCall(
this.votesTokenContract!,
return buildContractCallViem(
VotesERC20Abi,
this.votesTokenContractAddress,
'approve',
[this.predictedTokenClaimAddress, azoriusGovernanceDaoData.parentAllocationAmount],
0,
Expand Down Expand Up @@ -547,6 +553,10 @@ export class AzoriusTxBuilder extends BaseTxBuilder {
}

private setContracts() {
if (!this.predictedTokenAddress) {
return;
}
Comment on lines +556 to +558
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this defined when existing token is used for DAO Creation? Ie set to the existing token's address

Copy link
Member Author

Choose a reason for hiding this comment

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

TBH not sure. I assumed (perhaps incorrectly?) that this check here was safe, because if you look at the old code on line 561, we were forcing the value to be valid (this.predictedTokenAddress!).

I could just put that ! back in on new line 574 and remove this conditional early exit, but I don't like ! in typescript.

I suppose I should go and manually test this out, huh

Copy link
Member Author

Choose a reason for hiding this comment

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

In the case when a user brings their own token to new Azorius DAO creation, this.predictedTokenAddress gets set to that existing token address. So, this function executes as expected. Thanks for the call out here.


const daoData = this.daoData as AzoriusGovernanceDAO;
this.azoriusContract = Azorius__factory.connect(
this.predictedAzoriusAddress!,
Expand All @@ -557,10 +567,7 @@ export class AzoriusTxBuilder extends BaseTxBuilder {
this.predictedStrategyAddress!,
this.signerOrProvider,
);
this.votesTokenContract = VotesERC20__factory.connect(
this.predictedTokenAddress!,
this.signerOrProvider,
);
this.votesTokenContractAddress = this.predictedTokenAddress;
} else if (daoData.votingStrategyType === VotingStrategyType.LINEAR_ERC721) {
this.linearERC721VotingContract = LinearERC721Voting__factory.connect(
this.predictedStrategyAddress!,
Expand Down
4 changes: 4 additions & 0 deletions src/models/BaseTxBuilder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { ethers } from 'ethers';
import { PublicClient } from 'viem';
import {
BaseContracts,
AzoriusContracts,
Expand All @@ -10,6 +11,7 @@ import {

export class BaseTxBuilder {
protected readonly signerOrProvider: ethers.Signer | any;
protected readonly publicClient: PublicClient;
protected readonly baseContracts: BaseContracts;
protected readonly azoriusContracts: AzoriusContracts | undefined;
protected readonly daoData: SafeMultisigDAO | AzoriusERC20DAO | AzoriusERC721DAO | SubDAO;
Expand All @@ -18,13 +20,15 @@ export class BaseTxBuilder {

constructor(
signerOrProvider: ethers.Signer | any,
publicClient: PublicClient,
baseContracts: BaseContracts,
azoriusContracts: AzoriusContracts | undefined,
daoData: SafeMultisigDAO | AzoriusERC20DAO | AzoriusERC721DAO | SubDAO,
parentAddress?: string,
parentTokenAddress?: string,
) {
this.signerOrProvider = signerOrProvider;
this.publicClient = publicClient;
this.baseContracts = baseContracts;
this.daoData = daoData;
this.azoriusContracts = azoriusContracts;
Expand Down
10 changes: 8 additions & 2 deletions src/models/DaoTxBuilder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { ethers } from 'ethers';
import { zeroAddress } from 'viem';
import { PublicClient, zeroAddress } from 'viem';
import { GnosisSafeL2 } from '../assets/typechain-types/usul/@gnosis.pm/safe-contracts/contracts';
import { buildContractCall, encodeMultiSend } from '../helpers';
import {
Expand Down Expand Up @@ -34,6 +34,7 @@ export class DaoTxBuilder extends BaseTxBuilder {

constructor(
signerOrProvider: ethers.Signer | any,
publicClient: PublicClient,
baseContracts: BaseContracts,
azoriusContracts: AzoriusContracts | undefined,
daoData: SafeMultisigDAO | AzoriusERC20DAO | AzoriusERC721DAO,
Expand All @@ -48,6 +49,7 @@ export class DaoTxBuilder extends BaseTxBuilder {
) {
super(
signerOrProvider,
publicClient,
baseContracts,
azoriusContracts,
daoData,
Expand Down Expand Up @@ -131,9 +133,13 @@ export class DaoTxBuilder extends BaseTxBuilder {
// If subDAO and parentAllocation, deploy claim module
let tokenClaimTx: SafeTransaction | undefined;
const parentAllocation = (this.daoData as AzoriusERC20DAO).parentAllocationAmount;

if (this.parentTokenAddress && parentAllocation && parentAllocation !== 0n) {
tokenClaimTx = azoriusTxBuilder.buildDeployTokenClaim();
const tokenApprovalTx = azoriusTxBuilder.buildApproveClaimAllocation();
if (!tokenApprovalTx) {
throw new Error('buildApproveClaimAllocation returned undefined');
}
tokenClaimTx = azoriusTxBuilder.buildDeployTokenClaim();
this.internalTxs.push(tokenApprovalTx);
}

Expand Down
3 changes: 3 additions & 0 deletions src/models/FreezeGuardTxBuilder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
encodeAbiParameters,
parseAbiParameters,
isHex,
PublicClient,
} from 'viem';
import { GnosisSafeL2 } from '../assets/typechain-types/usul/@gnosis.pm/safe-contracts/contracts';
import { buildContractCall } from '../helpers';
Expand Down Expand Up @@ -62,6 +63,7 @@ export class FreezeGuardTxBuilder extends BaseTxBuilder {

constructor(
signerOrProvider: any,
publicClient: PublicClient,
baseContracts: BaseContracts,
daoData: SubDAO,
safeContract: GnosisSafeL2,
Expand All @@ -76,6 +78,7 @@ export class FreezeGuardTxBuilder extends BaseTxBuilder {
) {
super(
signerOrProvider,
publicClient,
baseContracts,
azoriusContracts,
daoData,
Expand Down
7 changes: 6 additions & 1 deletion src/models/TxBuilderFactory.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { ethers } from 'ethers';
import { getAddress } from 'viem';
import { PublicClient, getAddress } from 'viem';
import { GnosisSafeL2 } from '../assets/typechain-types/usul/@gnosis.pm/safe-contracts/contracts';
import { GnosisSafeL2__factory } from '../assets/typechain-types/usul/factories/@gnosis.pm/safe-contracts/contracts';
import { getRandomBytes } from '../helpers';
Expand Down Expand Up @@ -34,6 +34,7 @@ export class TxBuilderFactory extends BaseTxBuilder {

constructor(
signerOrProvider: ethers.Signer | any,
publicClient: PublicClient,
baseContracts: BaseContracts,
azoriusContracts: AzoriusContracts | undefined,
daoData: SafeMultisigDAO | AzoriusERC20DAO | AzoriusERC721DAO | SubDAO,
Expand All @@ -45,6 +46,7 @@ export class TxBuilderFactory extends BaseTxBuilder {
) {
super(
signerOrProvider,
publicClient,
baseContracts,
azoriusContracts,
daoData,
Expand Down Expand Up @@ -86,6 +88,7 @@ export class TxBuilderFactory extends BaseTxBuilder {
): DaoTxBuilder {
return new DaoTxBuilder(
this.signerOrProvider,
this.publicClient,
this.baseContracts,
this.azoriusContracts,
this.daoData,
Expand All @@ -108,6 +111,7 @@ export class TxBuilderFactory extends BaseTxBuilder {
): FreezeGuardTxBuilder {
return new FreezeGuardTxBuilder(
this.signerOrProvider,
this.publicClient,
this.baseContracts,
this.daoData as SubDAO,
this.safeContract!,
Expand All @@ -133,6 +137,7 @@ export class TxBuilderFactory extends BaseTxBuilder {
public async createAzoriusTxBuilder(): Promise<AzoriusTxBuilder> {
const azoriusTxBuilder = new AzoriusTxBuilder(
this.signerOrProvider,
this.publicClient,
this.baseContracts,
this.azoriusContracts!,
this.daoData as AzoriusERC20DAO,
Expand Down