Skip to content
This repository has been archived by the owner on Jan 22, 2024. It is now read-only.

Xpring ESLint - Round 3 #282

Merged
merged 4 commits into from
May 20, 2020
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 25 additions & 25 deletions src/Common/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ export interface ClassicAddress {
test: boolean
}

abstract class Utils {
const utils = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change from a class to an object that holds functions.

This is the proper fix for the no-extraneous-class rule. I did ask about using abstract classes for this problem, but the consensus seems to be to just use EcmaScript modules.

It's a little odd that we're exporting an object as opposed to the individual functions, but this keeps us consistent with the old way of exporting the class.

/**
* Validate that the given string is a valid address for the XRP Ledger.
*
Expand All @@ -34,12 +34,12 @@ abstract class Utils {
* @param address - An address to check.
* @returns True if the address is valid, otherwise false.
*/
public static isValidAddress(address: string): boolean {
isValidAddress(address: string): boolean {
return (
addressCodec.isValidClassicAddress(address) ||
addressCodec.isValidXAddress(address)
)
}
},

/**
* Validate whether the given string is a valid X-address for the XRP Ledger.
Expand All @@ -49,9 +49,9 @@ abstract class Utils {
* @param address - An address to check.
* @returns True if the address is a valid X-address, otherwise false.
*/
public static isValidXAddress(address: string): boolean {
isValidXAddress(address: string): boolean {
return addressCodec.isValidXAddress(address)
}
},

/**
* Validate whether the given string is a valid classic address for the XRP Ledger.
Expand All @@ -61,9 +61,9 @@ abstract class Utils {
* @param address - An address to check.
* @returns True if the address is a valid classic address, otherwise false.
*/
public static isValidClassicAddress(address: string): boolean {
isValidClassicAddress(address: string): boolean {
return addressCodec.isValidClassicAddress(address)
}
},

/**
* Encode the given classic address and tag into an x-address.
Expand All @@ -75,7 +75,7 @@ abstract class Utils {
* @param test - Whether the address is for use on a test network, defaults to `false`.
* @returns A new x-address if inputs were valid, otherwise undefined.
*/
public static encodeXAddress(
encodeXAddress(
classicAddress: string,
tag: number | undefined,
test = false,
Expand All @@ -85,13 +85,13 @@ abstract class Utils {
}

// Xpring Common JS's API takes a string|undefined while the underlying address library wants string|false.
const shimTagParameter = tag !== undefined ? tag : false
const shimTagParameter = tag ?? false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's an ESLint rule against if(negative) {...} else {...}, because there's usually a better way to write it (like here).

return addressCodec.classicAddressToXAddress(
classicAddress,
shimTagParameter,
test,
)
}
},

/**
* Decode a `ClassicAddress` from a given x-address.
Expand All @@ -101,7 +101,7 @@ abstract class Utils {
* @param xAddress - The xAddress to decode.
* @returns A `ClassicAddress`
*/
public static decodeXAddress(xAddress: string): ClassicAddress | undefined {
decodeXAddress(xAddress: string): ClassicAddress | undefined {
if (!addressCodec.isValidXAddress(xAddress)) {
return undefined
}
Expand All @@ -110,41 +110,41 @@ abstract class Utils {
return {
address: shimClassicAddress.classicAddress,
tag:
shimClassicAddress.tag !== false ? shimClassicAddress.tag : undefined,
shimClassicAddress.tag === false ? undefined : shimClassicAddress.tag,
test: shimClassicAddress.test,
}
}
},

/**
* Convert the given byte array to a hexadecimal string.
*
* @param bytes - An array of bytes
* @returns An encoded hexadecimal string.
*/
public static toHex(bytes: Uint8Array): string {
toHex(bytes: Uint8Array): string {
return Buffer.from(bytes).toString('hex').toUpperCase()
}
},

/**
* Convert the given hexadecimal string to a byte array.
*
* @param hex - A hexadecimal string.
* @returns A decoded byte array.
*/
public static toBytes(hex: string): Uint8Array {
toBytes(hex: string): Uint8Array {
return Uint8Array.from(Buffer.from(hex, 'hex'))
}
},

/**
* Convert the given transaction blob to a transaction hash.
*
* @param transactionBlobHex - A hexadecimal encoded transaction blob.
* @returns A hex encoded hash if the input was valid, otherwise undefined.
*/
public static transactionBlobToTransactionHash(
transactionBlobToTransactionHash(
transactionBlobHex: string,
): string | undefined {
if (!Utils.isHex(transactionBlobHex)) {
if (!this.isHex(transactionBlobHex)) {
return undefined
}

Expand All @@ -153,32 +153,32 @@ abstract class Utils {
)
const hash = this.sha512Half(prefixedTransactionBlob)
return this.toHex(hash)
}
},

/**
* Check if the given string is valid hex.
*
* @param input - The input to check.
* @returns true if the input is valid hex, otherwise false.
*/
public static isHex(input: string): boolean {
isHex(input: string): boolean {
const hexRegEx = /(?:[0-9]|[a-f])/gimu
return (input.match(hexRegEx) ?? []).length === input.length
}
},

/**
* Compute the SHA512 half hash of the given bytes.
*
* @param input - The input to hash.
* @returns The hash of the input.
*/
private static sha512Half(bytes: Uint8Array): Uint8Array {
sha512Half(bytes: Uint8Array): Uint8Array {
const sha512 = createHash('sha512')
const hashHex = sha512.update(bytes).digest('hex').toUpperCase()
const hash = this.toBytes(hashHex)

return hash.slice(0, hash.length / 2)
}
},
}

export default Utils
export default utils
21 changes: 13 additions & 8 deletions src/PayID/pay-id-components.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,19 @@
* A class which encapsulates components of a Pay ID.
*/
export default class PayIdComponents {
public readonly host: string
public readonly path: string

/**
* Create a new PayIdComponents.
*
* @param host - The host of the payment pointer.
* @param path - The path of the payment pointer, starting with a leading '/'.
*/
public constructor(
public readonly host: string,
public readonly path: string,
) {}
public constructor(host: string, path: string) {
this.host = host
this.path = path
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a rule that disallows declaring class properties in the constructor. I'm ok with disabling this rule for now, but since some properties might not be declarable in the constructor, it makes sense to be consistent.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm happy to use lint rules if that's what folks like doing.

Copy link
Contributor

Choose a reason for hiding this comment

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

The only reason I could think of not liking this style is that you forget to assign a variable. However, this appears to not be the case, since the following errors:

public constructor(host: string, path: string) {
    this.host = host
}

Yells taht I didn't initialize path.

}
}

/**
Expand All @@ -24,14 +27,16 @@ export default class PayIdComponents {
* @deprecated Please use the idiomatically named `PayIdComponents` class instead.
*/
export class PayIDComponents {
public readonly host: string
public readonly path: string
/**
* Create a new PayIDComponents.
*
* @param host - The host of the payment pointer.
* @param path - The path of the payment pointer, starting with a leading '/'.
*/
public constructor(
public readonly host: string,
public readonly path: string,
) {}
public constructor(host: string, path: string) {
this.host = host
this.path = path
}
}
38 changes: 17 additions & 21 deletions src/PayID/pay-id-utils.ts
Original file line number Diff line number Diff line change
@@ -1,39 +1,37 @@
// Disable multiple classes to accommodate the switch to idiomatic style naming.
// TODO(keefertaylor): Remove this when migration is complete.
/* eslint-disable max-classes-per-file */

/* eslint-disable import/no-deprecated */
import PayIdComponents, { PayIDComponents } from './pay-id-components'

/**
* A static utility class for Pay ID functionality.
*/
export default class PayIdUtils {
const payIdUtils = {
/**
* Parse a PayID string to a set of PayIDComponents object
*
* @param parsePayID - The input Pay ID.
* @param payID - The input Pay ID.
* @returns A PayIDComponents object if the input was valid, otherwise undefined.
*/
public static parsePayID(payID: string): PayIdComponents | undefined {
if (!PayIdUtils.isASCII(payID)) {
return
parsePayID(payID: string): PayIdComponents | undefined {
if (!this.isASCII(payID)) {
return undefined
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The consistent-return rule requires being explicit with your return statements if your function contains a return statement that returns something other than undefined (like this function with return new PayIdComponents().

Copy link
Contributor

Choose a reason for hiding this comment

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

woo. I actually prefer this.

}

// Validate there are two components of a payment pointer.
const components = payID.split('$')
if (components.length !== 2) {
return
return undefined
}

// Validate the host and path have values.
const path = components[0]
const host = components[1]
if (host.length === 0 || path.length === 0) {
return
return undefined
}

return new PayIdComponents(host, `/${path}`)
}
},

/**
* Validate if the input is ASCII based text.
Expand All @@ -44,32 +42,30 @@ export default class PayIdUtils {
* @param input - The input to check
* @returns A boolean indicating the result.
*/
private static isASCII(input: string): boolean {
isASCII(input: string): boolean {
// eslint-disable-next-line no-control-regex
return /^[\x00-\x7F]*$/u.test(input)
}
},
}
export default payIdUtils

/**
* A static utility class for PayID.
*
* @deprecated Please use the idiomatically named `PayIdUtils` class instead.
*/
export class PayIDUtils {
// eslint-disable-next-line @typescript-eslint/no-extraneous-class
export abstract class PayIDUtils {
/**
* Parse a PayID string to a set of PayIDComponents object
*
* @param parsePayID - The input Pay ID.
* @returns A PayIDComponents object if the input was valid, otherwise undefined.
*/
public static parsePayID(payID: string): PayIDComponents | undefined {
const components = PayIdUtils.parsePayID(payID)
return components !== undefined
? new PayIDComponents(components?.host, components?.path)
const components = payIdUtils.parsePayID(payID)
return components
? new PayIDComponents(components.host, components.path)
: undefined
}

/** Please do not instantiate this static utility class. */
// eslint-disable-next-line @typescript-eslint/no-empty-function
private constructor() {}
}
Loading