Remove erc20claim typechain #1665
Conversation
✅ Deploy Preview for fractal-dev ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
adamgall
force-pushed
the
remove-LockRelease-typechain
branch
from
72d36ee
to
3b73b12
Compare
adamgall
force-pushed
the
remove-erc20claim-typechain
branch
from
e1e4d75
to
1c0f337
Compare
adamgall
force-pushed
the
remove-LockRelease-typechain
branch
from
3b73b12
to
69cf3cf
Compare
adamgall
force-pushed
the
remove-erc20claim-typechain
branch
from
1c0f337
to
ea82775
Compare
adamgall
force-pushed
the
remove-LockRelease-typechain
branch
from
69cf3cf
to
c2491d5
Compare
adamgall
force-pushed
the
remove-erc20claim-typechain
branch
from
ea82775
to
a61be02
Compare
adamgall
force-pushed
the
remove-LockRelease-typechain
branch
from
c2491d5
to
e0c8802
Compare
adamgall
force-pushed
the
remove-erc20claim-typechain
branch
from
a61be02
to
8468002
Compare
| return; | ||
| } | ||
| // action to governance | ||
| action.dispatch({ | ||
| type: FractalGovernanceAction.SET_CLAIMING_CONTRACT, | ||
| payload: possibleTokenClaimContract, | ||
| payload: getAddress(approvals[0].args.spender), |
There was a problem hiding this comment.
getAddress(approvals[0].args.spender used twice - let's put that into variable
There was a problem hiding this comment.
Actually, we don't need the getAddress wrap here anyway! approvals[0].args.spender is already Address type.
| .queryFilter(tokenClaimFilter) | ||
| const possibleTokenClaimContract = getContract({ | ||
| abi: ERC20ClaimAbi, | ||
| address: getAddress(approvals[0].args.spender), |
There was a problem hiding this comment.
Same: can just remove that completely!
| @@ -78,6 +80,7 @@ export class AzoriusTxBuilder extends BaseTxBuilder { | |||
| votesERC20MasterCopyAddress: string, | |||
| moduleProxyFactoryAddress: Address, | |||
| multiSendCallOnlyAddress: Address, | |||
| erc20ClaimMasterCopyAddress: Address, | |||
There was a problem hiding this comment.
The more I'm looking into this - the more I feel like we don't need this whole drilling and we can just create some helper function that will return these addresses for the given chain ID.
And then we just need to pass chainId down here. But yeah - not a point for this specific PR
There was a problem hiding this comment.
Totally agree. All of this code in these sets of PRs just incrementally transition from one way of doing things to another. After these PRs are complete, I'm going to do another set of "clean up" PRs to deal with refactoring and DRYing up all of the shit that was introduced as we switched from one system to another.
| @@ -178,6 +181,7 @@ export class TxBuilderFactory extends BaseTxBuilder { | |||
| this.votesERC20MasterCopyAddress, | |||
| getAddress(this.moduleProxyFactoryAddress), | |||
| getAddress(this.multiSendCallOnlyAddress), | |||
| getAddress(this.erc20ClaimMasterCopyAddress), | |||
There was a problem hiding this comment.
Seems like we kinda relying on getAddress to throw here and thus type-casting this.erc20ClaimMasterCopyAddress to Address.
I'd rather prefer more explicit approach -
if (!isAddress(this.erc20ClaimMasterCopyAddress)) {
throw new Error('Fuck you, some shit is not address, but we expect that to be. Go cry or smthng, we don't care');
}
But ideally we should be displaying proper error fallback to the user - so we should be catching the error before.
I also assume it might be the case that after bunch of PRs these addresses would actually arrive here as Address type validated - so feel free to ignore all this text if that's the intent and this is just temporary solution 😄
There was a problem hiding this comment.
Yep, your assumption is correct. After all of these PRs are finished, the intention is that these addresses are typed correctly as Address at the source, and I'll be able to comb through the codebase removing all of these getAddress() calls.
So, leaving for now, with the understanding that this will be cleaned up later.
For your reference, I have a text doc with a list of shit I will need to go back and clean up after these PRs are finished. It currently is:
cleanup:
- buildContractCallViem
- contractCallViem
- all the getAddress usages
- TX Builder dependency injection wrangling
- standardize contract names in networkconfig
…nly store an address, not a whole contract object
adamgall
force-pushed
the
remove-erc20claim-typechain
branch
from
8468002
to
699148d
Compare
Please review and merge #1664 first.