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.

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

refactor: Simplify inventory client parsing #963

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

pxrl
Copy link
Contributor

@pxrl pxrl commented Oct 2, 2023

This helps to drop an UMA dependency, and is arguably nicer because it doesn't corce the js Object to and from a string type.

@pxrl
Copy link
Contributor Author

pxrl commented Oct 2, 2023

replaceAddressCase() taken from here: https://github.com/UMAprotocol/protocol/blob/e9af17eb2985f482635b9009a1174a694a025d31/packages/common/src/FormattingUtils.ts#L155

In general I'm not a fan of the former implementation because it coerces an object into a string in order to achieve address normalisation into checksum case, so I opted to operate directly on the string here.

@pxrl pxrl force-pushed the pxrl/parseAddress branch 2 times, most recently from f0d582d to 4656e88 Compare October 2, 2023 15:43
@@ -65,11 +65,12 @@ export class RelayerConfig extends CommonConfig {
this.slowDepositors = SLOW_DEPOSITORS
? JSON.parse(SLOW_DEPOSITORS).map((depositor) => ethers.utils.getAddress(depositor))
: [];
this.inventoryConfig = RELAYER_INVENTORY_CONFIG ? JSON.parse(RELAYER_INVENTORY_CONFIG) : {};
this.inventoryConfig = JSON.parse(
RELAYER_INVENTORY_CONFIG?.replace(/0x[a-fA-F0-9]{40}/g, ethers.utils.getAddress) ?? "{}"
Copy link
Member

Choose a reason for hiding this comment

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

Do you think we should make this a helper function? I think there are several other places where we expect an address string set in the .env that might not be checksummed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can make it a utility function; I only did it here because this was the only place I could see that we actually normalised addressed on input.

I'll update accordingly 👍

@@ -65,11 +65,12 @@ export class RelayerConfig extends CommonConfig {
this.slowDepositors = SLOW_DEPOSITORS
? JSON.parse(SLOW_DEPOSITORS).map((depositor) => ethers.utils.getAddress(depositor))
: [];
this.inventoryConfig = RELAYER_INVENTORY_CONFIG ? JSON.parse(RELAYER_INVENTORY_CONFIG) : {};
this.inventoryConfig = JSON.parse(
RELAYER_INVENTORY_CONFIG?.replace(/0x[a-fA-F0-9]{40}/g, ethers.utils.getAddress) ?? "{}"
Copy link
Contributor

Choose a reason for hiding this comment

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

It may be easier to use a reviver in this case. JSON.parse's second parameter is an optional function that looks at keys/values. We could probably create a reviver function like:

/**
 * Converts a potential string/array of strings that match an address into a checksummed address
 **/
export function checksumAddress(value: unknown): unknown {
    const isArray = Array.isArray(value);
    const result: unknown[] = (isArray ? value : [value]).map((v) => utils.isAddress(v) ? utils.getAddress(v) : v;
    return isArray ? result : result[0];
}
JSON.parse(
    RELAYER_INVENTORY_CONFIG ?? {},
    (, v) => checksumAddress(v)
)

Copy link
Contributor

@james-a-morris james-a-morris left a comment

Choose a reason for hiding this comment

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

Left a comment to possibly add discussion about the readability of the parse function

This helps to drop an UMA dependency, and is arguably nicer because it
doesn't corce the js Object to and from a string type.
@pxrl pxrl marked this pull request as draft October 5, 2023 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants