Bugfix/abi encoding #1609
Bugfix/abi encoding #1609
Conversation
…do work currently, the code is just misleading
✅ Deploy Preview for fractal-dev ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| @@ -25,7 +25,6 @@ import { generateContractByteCodeLinear, generateSalt } from './helpers/utils'; | |||
|
|
|||
| export class AzoriusTxBuilder extends BaseTxBuilder { | |||
| private readonly safeContract: GnosisSafeL2; | |||
| private readonly predictedSafeAddress: string; | |||
| @@ -54,7 +53,6 @@ export class AzoriusTxBuilder extends BaseTxBuilder { | |||
| azoriusContracts: AzoriusContracts, | |||
| daoData: AzoriusERC20DAO | AzoriusERC721DAO, | |||
| safeContract: GnosisSafeL2, | |||
| predictedSafeAddress: string, | |||
| @@ -68,7 +66,6 @@ export class AzoriusTxBuilder extends BaseTxBuilder { | |||
| ); | |||
|
|
|||
| this.safeContract = safeContract; | |||
| this.predictedSafeAddress = predictedSafeAddress; | |||
| @@ -130,7 +130,6 @@ export class TxBuilderFactory extends BaseTxBuilder { | |||
| this.azoriusContracts!, | |||
| this.daoData as AzoriusERC20DAO, | |||
| this.safeContract!, | |||
| this.predictedSafeAddress!, | |||
src/models/AzoriusTxBuilder.ts
Outdated
| ['uint32', 'address', 'address', 'address', 'uint256'], | ||
| [ | ||
| 0, // deadlineBlock. do we capture this in the UI? |
There was a problem hiding this comment.
This seems like an actual bug. The contract is expecting a uint32 before the rest of those parameters, and we weren't sending one in. Is this code path just rarely (ever) used in the app? I need to do some manual testing still.
There was a problem hiding this comment.
Yeah, sure enough this (original code) is BROKEN. Can't execute a proposal in which we're creating a subdao with a token claim for parent holders.
There was a problem hiding this comment.
And I AM able to execute the same kind of "parent holder token claim" proposal with my changes here in place. Nice.
| @@ -217,7 +217,7 @@ export class FreezeGuardTxBuilder extends BaseTxBuilder { | |||
| 'setUp', | |||
| [ | |||
| ethers.utils.defaultAbiCoder.encode( | |||
| ['uint256', 'uint256', 'address', 'address', 'address'], | |||
| ['uint32', 'uint32', 'address', 'address', 'address'], | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Did manual testing, this works as expected.
| this.freezeGuardCallData = AzoriusFreezeGuard__factory.createInterface().encodeFunctionData( | ||
| 'setUp', | ||
| [ | ||
| ethers.utils.defaultAbiCoder.encode( | ||
| ['address', 'address', 'address', 'address', 'uint256'], | ||
| ['address', 'address'], |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Did manual testing, this works as expected.
i think i found
in various places where we do ABI encoding