[WIP] improve(utils): Avoid recomputing relayData hashes #657
Conversation
This will save redundant recomputation cycles of a relayData hash when it is needed more than once.
| @@ -19,6 +19,7 @@ export interface RelayData { | |||
| fillDeadline: number; | |||
| exclusiveRelayer: string; | |||
| exclusivityDeadline: number; | |||
| _hash?: Record<number, string>; | |||
There was a problem hiding this comment.
It'd be a fair bit more efficient for this to be a simple string, but the RelayData type doesn't include the destinationChainId, so the same RelayData object can map to multiple hashes depending on the value of destinationChainId. In the interests of safety it seems best to use a record, but I'd love to find a way to use a string instead.
There was a problem hiding this comment.
Does that mean the hash really should be one level higher where the destination chain is present (i.e. the hash should be in fill and deposit)?
There was a problem hiding this comment.
Yeah, that's a good option. It introduces a little bit of duplication but it's nothing major. I'll think about another way of passing in the hash if it's not defined in RelayData.
| const _hash = undefined; | ||
|
|
||
| // Invalid input amount. | ||
| expect(validateFillForDeposit({ ...validFill, inputAmount: toBNWei(1337) }, deposit_2)).to.be.false; | ||
| expect(validateFillForDeposit({ ...validFill, inputAmount: toBNWei(1337), _hash }, deposit_2)).to.be.false; | ||
|
|
||
| // Changed the output token. | ||
| expect(validateFillForDeposit(validFill, { ...deposit_2, outputToken: owner.address })).to.be.false; | ||
| expect(validateFillForDeposit(validFill, { ...deposit_2, outputToken: owner.address, _hash })).to.be.false; | ||
|
|
||
| // Changed the output amount. | ||
| expect(validateFillForDeposit({ ...validFill, outputAmount: toBNWei(1337) }, deposit_2)).to.be.false; | ||
| expect(validateFillForDeposit({ ...validFill, outputAmount: toBNWei(1337), _hash }, deposit_2)).to.be.false; | ||
|
|
||
| // Invalid depositId. | ||
| expect(validateFillForDeposit({ ...validFill, depositId: 1337 }, deposit_2)).to.be.false; | ||
| expect(validateFillForDeposit({ ...validFill, depositId: 1337, _hash }, deposit_2)).to.be.false; | ||
|
|
||
| // Changed the depositor. | ||
| expect(validateFillForDeposit({ ...validFill, depositor: relayer.address }, deposit_2)).to.be.false; | ||
| expect(validateFillForDeposit({ ...validFill, depositor: relayer.address, _hash }, deposit_2)).to.be.false; | ||
|
|
||
| // Changed the recipient. | ||
| expect(validateFillForDeposit({ ...validFill, recipient: relayer.address }, deposit_2)).to.be.false; | ||
| expect(validateFillForDeposit({ ...validFill, recipient: relayer.address, _hash }, deposit_2)).to.be.false; |
There was a problem hiding this comment.
These tests exposed a sharp edge - producing a new relayData event based on an existing one, with some modified fields, risks inadvertently copying the old _hash. It needs to be manually unset. It's a pretty big foot gun...
| @@ -19,6 +19,7 @@ export interface RelayData { | |||
| fillDeadline: number; | |||
| exclusiveRelayer: string; | |||
| exclusivityDeadline: number; | |||
| _hash?: Record<number, string>; | |||
There was a problem hiding this comment.
Does that mean the hash really should be one level higher where the destination chain is present (i.e. the hash should be in fill and deposit)?
This will save redundant recomputation cycles of a relayData hash when it is needed more than once.
This PR will be updated with tests.