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

Bugfix/abi encoding #1609

Merged
merged 4 commits into from
Apr 30, 2024
Merged

Bugfix/abi encoding #1609

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
66ae3a0
Remove unused variable
adamgall Apr 26, 2024
2df103f
Add missing abi parameter on "setUp" encoding for ERC20Claim
adamgall Apr 26, 2024
e0b1126
Clean up ABI encoding for a couple of contract calls, but I bet they …
adamgall Apr 26, 2024
667bb65
Clean up a comment on a magic number
adamgall Apr 27, 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
6 changes: 2 additions & 4 deletions src/models/AzoriusTxBuilder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import { generateContractByteCodeLinear, generateSalt } from './helpers/utils';

export class AzoriusTxBuilder extends BaseTxBuilder {
private readonly safeContract: GnosisSafeL2;
private readonly predictedSafeAddress: string;
Copy link
Member Author

Choose a reason for hiding this comment

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

Unused, removing.


private encodedSetupTokenData: string | undefined;
private encodedSetupERC20WrapperData: string | undefined;
Expand Down Expand Up @@ -54,7 +53,6 @@ export class AzoriusTxBuilder extends BaseTxBuilder {
azoriusContracts: AzoriusContracts,
daoData: AzoriusERC20DAO | AzoriusERC721DAO,
safeContract: GnosisSafeL2,
predictedSafeAddress: string,
Copy link
Member Author

Choose a reason for hiding this comment

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

Unused, removing.

parentAddress?: string,
parentTokenAddress?: string,
) {
Expand All @@ -68,7 +66,6 @@ export class AzoriusTxBuilder extends BaseTxBuilder {
);

this.safeContract = safeContract;
this.predictedSafeAddress = predictedSafeAddress;
Copy link
Member Author

Choose a reason for hiding this comment

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

Unused, removing.


this.tokenNonce = getRandomBytes();
this.claimNonce = getRandomBytes();
Expand Down Expand Up @@ -349,8 +346,9 @@ export class AzoriusTxBuilder extends BaseTxBuilder {
private setEncodedSetupTokenClaimData() {
const azoriusGovernanceDaoData = this.daoData as AzoriusERC20DAO;
const encodedInitTokenData = defaultAbiCoder.encode(
['address', 'address', 'address', 'uint256'],
['uint32', 'address', 'address', 'address', 'uint256'],
[
0, // deadlineBlock. We don't capture this in the UI. 0 means no deadline to claim.
this.safeContract.address,
this.parentTokenAddress,
this.predictedTokenAddress,
Expand Down
9 changes: 2 additions & 7 deletions src/models/FreezeGuardTxBuilder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ export class FreezeGuardTxBuilder extends BaseTxBuilder {
'setUp',
[
ethers.utils.defaultAbiCoder.encode(
['uint256', 'uint256', 'address', 'address', 'address'],
['uint32', 'uint32', 'address', 'address', 'address'],
Copy link
Member Author

@adamgall adamgall Apr 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The contract is expecting uint32s here, not uint256s.

Does this currently work as expected? I bet so, because we have tested freeze guard things before.

Will it be a problem to change this? Hope not.

subDaoData.timelockPeriod and subDaoData.executionPeriod are both typed as bigint though, which also "seems wrong" to me, because those things are uint32s on the contract. They don't need to be bigints, they can just be numbers. Updating those types makes this PR much hairier though.

I need to do some manual testing to make sure this change still works as intended, just like this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Did manual testing, this works as expected.

[
subDaoData.timelockPeriod, // Timelock Period
subDaoData.executionPeriod, // Execution Period
Expand All @@ -231,19 +231,14 @@ export class FreezeGuardTxBuilder extends BaseTxBuilder {
}

private setFreezeGuardCallDataAzorius() {
const subDaoData = this.daoData as SubDAO;

this.freezeGuardCallData = AzoriusFreezeGuard__factory.createInterface().encodeFunctionData(
'setUp',
[
ethers.utils.defaultAbiCoder.encode(
['address', 'address', 'address', 'address', 'uint256'],
['address', 'address'],
Copy link
Member Author

@adamgall adamgall Apr 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We were straight up "encoding too much", here. The contract only expects two addresses. So, slimmed it down.

I bet this still works currently because the smart contract probably just discards everything after the first two address parameters.

Need to do some manual testing on this too of course.

Copy link
Member Author

Choose a reason for hiding this comment

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

Did manual testing, this works as expected.

[
this.parentAddress, // Owner -- Parent DAO
this.freezeVotingAddress, // Freeze Voting
this.strategyAddress, // Base Strategy
this.azoriusAddress, // Azorius
subDaoData.executionPeriod, // Execution Period
],
),
],
Expand Down
1 change: 0 additions & 1 deletion src/models/TxBuilderFactory.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,6 @@ export class TxBuilderFactory extends BaseTxBuilder {
this.azoriusContracts!,
this.daoData as AzoriusERC20DAO,
this.safeContract!,
this.predictedSafeAddress!,
Copy link
Member Author

Choose a reason for hiding this comment

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

Unused, removing.

this.parentAddress,
this.parentTokenAddress,
);
Expand Down