feat: start using a custom Address class #904
Conversation
Signed-off-by: bennett <[email protected]>
Signed-off-by: bennett <[email protected]>
Signed-off-by: bennett <[email protected]>
Signed-off-by: bennett <[email protected]>
Signed-off-by: bennett <[email protected]>
Signed-off-by: bennett <[email protected]>
Signed-off-by: bennett <[email protected]>
Signed-off-by: bennett <[email protected]>
| @@ -67,18 +67,20 @@ type DataCache = Record<string, Promise<LoadDataReturnValue>>; | |||
| // V3 dictionary helper functions | |||
| function updateExpiredDepositsV3(dict: ExpiredDepositsToRefundV3, deposit: V3DepositWithBlock): void { | |||
| // A deposit refund for a deposit is invalid if the depositor has a bytes32 address input for an EVM chain. It is valid otherwise. | |||
| if (chainIsEvm(deposit.originChainId) && !isValidEvmAddress(deposit.depositor)) { | |||
| if (chainIsEvm(deposit.originChainId) && !deposit.depositor.isValidEvmAddress()) { | |||
There was a problem hiding this comment.
wdyt about internalising the chain -> address type mapping within the Address class, such that we could write: !deposit.depositor.isValidOn(deposit.originChainId) ?
There was a problem hiding this comment.
Good call. Done here 16dbd7e
| return; | ||
| } | ||
| const { originChainId, inputToken } = deposit; | ||
| const { originChainId, inputToken: _inputToken } = deposit; | ||
| const inputToken = _inputToken.toString(); |
There was a problem hiding this comment.
@dohaki @gsteenkamp89 Can you think of any way we can make the toString automatic, so we can rely on it happening in the background when inputToken is supplied as a key to an Object? Not sure if that's even possible, but if not, this usage scenario represents a sharp edge that we'll probably run into.
| if (this.spokePoolClients[chainId] === undefined) { | ||
| return toBN(0); | ||
| } | ||
| return this.spokePoolClients[chainId] | ||
| .getDeposits() | ||
| .filter((deposit) => deposit.blockNumber > latestBlockToSearch && deposit.inputToken === l2Token) |
There was a problem hiding this comment.
@dohaki @gsteenkamp89 Are you aware of any eslint rule we could use to flag checking equality between Address instances? @bmzig has removed all known instances from this PR but it seems like something we'd trip on in future.
| return Address.from(value); | ||
| } catch (error) { | ||
| // In case of any error during conversion, return the original value | ||
| // This will lead to a validation error, as the resulting value won't match the expected Address type |
There was a problem hiding this comment.
Any thoughts on how to handle this safely?
There was a problem hiding this comment.
Not yet, although this is pretty similar to what we do with BigNumber types. Do you anticipate running into deserialization issues here?
| const FillTypeSS = number(); | ||
|
|
||
| const V3RelayDataSS = { | ||
| inputToken: string(), | ||
| inputToken: AddressType, |
There was a problem hiding this comment.
How does this manifest when posted to arweave? Do we encode as a stringified address or a stringified object?
There was a problem hiding this comment.
Will keep this open since I am not sure yet. I'd assume a string due to how to store BigNumber types, but I will check.
| const l1Token = this.getL1TokenForDeposit(deposit); | ||
| // Use the latest hub block number to find the L2 token counterpart. | ||
| return this.getL2TokenForL1TokenAtBlock(l1Token, l2ChainId, deposit.quoteBlockNumber); | ||
| } | ||
|
|
||
| l2TokenEnabledForL1Token(l1Token: string, destinationChainId: number): boolean { | ||
| return this.l1TokensToDestinationTokens?.[l1Token]?.[destinationChainId] != undefined; | ||
| l2TokenEnabledForL1Token(_l1Token: EvmAddress | string, destinationChainId: number): boolean { |
There was a problem hiding this comment.
Where do we pass in a string for this?
There was a problem hiding this comment.
We do it here, unfortunately.
sdk/src/clients/HubPoolClient.ts
Line 217 in fa658be
Signed-off-by: bennett <[email protected]>
Signed-off-by: bennett <[email protected]>
Signed-off-by: bennett <[email protected]>
Signed-off-by: bennett <[email protected]>
Signed-off-by: bennett <[email protected]>
Still need to add a lot of unit tests. There are a few concerns with this PR, notably
Addressclasses meaningfully slow down the bots? This will need to be tested..toStringand access it with.toAddress). In general, if the token is guaranteed to be a bytes20 address, which is true when the token is an L1 token (but not true if the token is an L2 token), then we set/access maps withtoAddress. Otherwise, we usetoString.