chore: utils errors refactor by bhflm · Pull Request #38 · Railgun-Community/shared-models

bhflm · 2025-01-14T01:48:56Z

What
Current is a stab to refactor existing utils/errors.ts,

Why

In an effort to make the sdk more readable, this provides an enhancement for legibility of the current errors file, without the need of hardcoding existing/new errors and make it more extendable for future releases.

How

Separate onto a constants.ts to categorize different kind of errors types.
Create util functions to match error strings and avoid repetition of several if statements with string matching
Create custom error classes to manipulate errors differently, and separate them between Railgun Contract errors and General custom errors.
Adds tests for edge cases such as undefined, and ascii characters.

…of errors

…atch them 1:1 to existing errros

… move constants to dedicated file, and separate types onto another dedicated file

mattgle · 2025-01-14T11:14:56Z

src/utils/errors/constants.ts

+
+export const STRING_PREFIX_AFTER_UNICODE_REPLACEMENT = 'y %';
+
+// eslint-disable-next-line no-useless-escape


is this really needed?

mattgle · 2025-01-14T11:15:48Z

src/utils/errors/error.ts

+const validAscii = (str: string) => {
+  return str.replace(
+    INVALID_ASCII_REGEX,
+    '',
+  );
+};


Suggested change

const validAscii = (str: string) => {

return str.replace(

INVALID_ASCII_REGEX,

'',

);

};

const validAscii = (str: string) => str.replace(INVALID_ASCII_REGEX, '');

mattgle

Just a few comments, not really a MUST, but improvements IMO

emilbayes · 2025-01-14T12:37:57Z

src/utils/errors/error.ts

+const isRailgunError = (cause: Error): boolean => cause.message.toLowerCase().includes('railgunsmartwallet')
+
+export const sanitizeError = (cause: Error): Error => {
+  if (isDefined(cause) && cause.message) {


Personally, since the "else" here is shorter, early return would make the logic easier to read, eg.

if (cause?.message == null) return new Error('Unknown error. Please try again.', { cause }); if (isRailgunError(cause)) { /* ... */ }

Another question is, do we even need to check if cause is defined and has a message property? The type here says it must be an Error, so we perhaps only need to check the message being defined

emilbayes · 2025-01-14T12:38:04Z

src/utils/errors/constants.ts

+export const STRING_PREFIX_AFTER_UNICODE_REPLACEMENT = 'y %';
+
+// eslint-disable-next-line no-useless-escape
+export const INVALID_ASCII_REGEX = /[^A-Za-z 0-9 \.,\?""!@#\$%\^&\*\(\)-_=\+;:<>\/\\\|\}\{\[\]`~]*/g;


A neat little trick (that begs a comment), but all the printable ascii characters are in the range from (space) to ~ (tilde), so the regex can be compressed to: /[^ -~]+/g. Is this better? I think so (with a comment explaining what it is) since it ensures no printable ascii chars were missed, despite being more obscure knowledge, where a list like the above requires you to check that all symbols were included.

emilbayes · 2025-01-14T12:38:45Z

src/utils/errors/constants.ts

@@ -0,0 +1,92 @@
+import { CustomErrorMapping } from "./types";
+
+export const STRING_PREFIX_AFTER_UNICODE_REPLACEMENT = 'y %';


Do we know what this means? Looking at the usage, I don't understand how the name of the constant here makes sense (I know you just migrated it from what it historically was)

given the case that we keep it, a different name would be better IMHO, SANITIZED_MESSAGE_SEPARATOR or similar

but now that I've mentioned, I think we could remove this and be shortened to something that looks like:

const errorMessage = sanitizeAscii(cause.message);

since we're already properly sanitizing ASCII, and we don't need that much string manipulation besides this right??

Yeah reading again I think either this string used to be placed somehow in the string or it's an artefact from something that emitted errors with that string, and someone replaced it. Anyhow, the purpose is unclear to me. Maybe @zy0n or @mattgle remember seeing this string in errors at some point?

I have no idea tbh, but @mesquka might know

emilbayes · 2025-01-14T12:39:13Z

src/utils/errors/error.ts

+import { STRING_PREFIX_AFTER_UNICODE_REPLACEMENT, RAILGUN_ERRORS, CUSTOM_ERRORS, INVALID_ASCII_REGEX } from './constants';
+import { isDefined } from '../util';
+
+const validAscii = (str: string) => {


Now that we are refactoring, perhaps we should give this a more fitting name, perhaps sanitizeAscii

src/utils/errors/error.ts

emilbayes · 2025-01-14T12:42:49Z

src/utils/index.ts

 export * from './compare';
 export * from './fallback-provider';
-export * from './error';
+export * from './errors/error';


Perhaps this is an "internal module" so changing the path is insignificant, but if something is importing the error formatter directly, this is a breaking change

…e inside cause since its type Error

emilbayes · 2025-01-16T15:21:37Z

src/utils/errors/constants.ts

+
+// Matches any characters that are NOT in the printable ASCII range (space to tilde)
+// Printable ASCII characters are in the range of 32 (space) to 126 (tilde)
+export const INVALID_ASCII_REGEX = /[^ -~]+/g;


Thinking a bit more about this regex, we also remove LF (ie \n). It was not in the original regex, but I wonder if we actually would want to keep new lines. If so, we can add it to the range as \n

lets keep it as it was before then 👍🏼

emilbayes · 2025-01-16T15:23:03Z

src/utils/index.ts

 export * from './compare';
 export * from './fallback-provider';
-export * from './error';
+export * from './errors';


Did you mean to call this error per my comment about a potential breaking change?

mattgle · 2025-03-26T15:34:17Z

What should we do with this?
CC: @emilbayes @bhflm

bhflm added 15 commits January 13, 2025 22:46

chore: add new RailgunContractError class to separate different kind …

ff865aa

…of errors

chore: add customError class

553e4ea

chore: add constants.ts to segregate errors acording to categories, m…

04faafc

…atch them 1:1 to existing errros

fix: export constants for shared usage

3df2ce8

chore: refactor errors.ts sanitizeError with a more legible approach,…

5052dc8

… move constants to dedicated file, and separate types onto another dedicated file

fix: test suite correct path

41c9377

chore: EOF

729c2e8

chore: move ascii to constants.ts file

a64b53e

chore: rename ascii regex to be self documenting

bb05caf

fix: correct message and rename for transaction underpriced

b8e3601

chore: remove comments

39a2ba4

chore: match refactor with old tests

01c1403

chore: add more tests with edge cases for undefined, and ascii chars

0c4fe1e

fix: remove unnecesary classes, but keep Error class

2a624f9

misc: remove unnecesary spaces

5741d5a

bhflm requested review from emilbayes, mattgle and zy0n January 14, 2025 01:48

mattgle reviewed Jan 14, 2025

View reviewed changes

emilbayes reviewed Jan 14, 2025

View reviewed changes

bhflm added 7 commits January 14, 2025 11:34

chore: early return for undefined message case, check just for messag…

0e4c319

…e inside cause since its type Error

chore: rename to sanitizeAscii, compress ascii regex

21b9a12

fix: encapsulate catch inside objectwhen throwing Error

4945425

fix: rename errors/error.ts to errors/index.ts

c9c4a03

fix: errors path for test suite

95af502

chore: remove unnecesary eslint rules

0b206e2

chore: check for cause message and handle

082de73

bhflm requested a review from emilbayes January 14, 2025 22:22

chore: rename to sanitizeAscii

d79855b

bhflm requested a review from mattgle January 14, 2025 22:40

emilbayes reviewed Jan 16, 2025

View reviewed changes

bhflm added 3 commits January 16, 2025 20:32

fix: do not remove newlines within ascii regex

08db455

fix: error directory rename

e88a4f4

fix: correct error dir test path

142ef1f

bhflm requested a review from emilbayes January 20, 2025 14:35

bhflm mentioned this pull request Jan 29, 2025

Fix usage of WebSocketProvider and absence of pollingInterval #41

Open


		export const STRING_PREFIX_AFTER_UNICODE_REPLACEMENT = 'y %';

		// eslint-disable-next-line no-useless-escape

		@@ -0,0 +1,92 @@
		import { CustomErrorMapping } from "./types";

		export const STRING_PREFIX_AFTER_UNICODE_REPLACEMENT = 'y %';

Conversation

bhflm commented Jan 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattgle Jan 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattgle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattgle commented Mar 26, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mattgle Jan 14, 2025 •

edited

Loading