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

logger, config, deprecate legacy tx #1302

Merged
merged 6 commits into from
Jan 29, 2025
Merged

Conversation

tabaktoni
Copy link
Collaborator

@tabaktoni tabaktoni commented Jan 28, 2025

Motivation and Resolution

Implement logger and config
deprecate warning for old transaction versions

Development related changes

Use logger for console logging
Use config for global configuration

Checklist:

  • Performed a self-review of the code
  • Rebased to the last commit of the target branch (or merged it into my branch)
  • Linked the issues which this PR resolves
  • Documented the changes in code (API docs will be generated automatically)
  • Updated the tests
  • All tests are passing

@tabaktoni tabaktoni changed the title Feat/deprecate legacy tx logger, config, deprecate legacy tx Jan 28, 2025
@tabaktoni tabaktoni marked this pull request as ready for review January 28, 2025 22:39
@tabaktoni tabaktoni requested a review from penovicp January 28, 2025 22:39
Copy link
Collaborator

@penovicp penovicp left a comment

Choose a reason for hiding this comment

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

Mostly a bunch of nitpicks, main one is probably the one concerning doc generation visibility, in general lgtm.

I would also suggest adding a doc entry for the Guides section, but that can also be added afterwards.

@@ -448,6 +449,11 @@ export class RpcChannel {
nonce: toHex(details.nonce),
},
});

logger.warn(
'You are using deprecated transaction version (V0,V1,V2)!, \n Update to the latest V3 transactions!',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
'You are using deprecated transaction version (V0,V1,V2)!, \n Update to the latest V3 transactions!',
'You are using a deprecated transaction version (V0,V1,V2)!\nUpdate to the latest V3 transactions!',

Might also make sense to move the message into a const enum so that it gets both defined in one place and inlined.

@@ -0,0 +1,63 @@
import { DEFAULT_GLOBAL_CONFIG } from './constants';

type ConfigData = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be worthwhile to interlace the keys with DEFAULT_GLOBAL_CONFIG for IDE autocompletion but we can revisit that in the future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nice catch, I managed to break this one too in the late-night refactor


public get<K extends keyof ConfigData>(
key: K,
defaultValue?: ConfigData[K]
Copy link
Collaborator

Choose a reason for hiding this comment

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

The defaultValue parameter looks like it doesn't provide much utility.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It can be useful sometimes; I'll leave that one.

/**
* Logging class providing different levels of log
*/
class Logger {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably makes sense to export so it is exposed to the documentation generator for the API section.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think I'll just add manual docs because it could confuse some users in this case.

try {
formattedMessage += `\n${JSON.stringify(data, null, 2)}`;
} catch (error) {
formattedMessage += `\n[JSON.stringify Error/Circular]`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might make sense to propagate the error message since stringify could also fail for e.g. bigint values in data.

Comment on lines 148 to 153
const logLevelStringKeys = Object.keys(LogLevelIndex).filter(
(v) => Number.isNaN(Number(v)) && v !== 'OFF'
);
const logLevels = logLevelStringKeys.filter((s) => {
return this.shouldLog(LogLevelIndex[s as LogLevel]);
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

The Number.isNaN(Number(v)) check doesn't seem necessary so the two filters could be merged.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Leftover after refactoring from enum to const pattern


export type LogLevelIndex = ValuesType<typeof LogLevelIndex>;

export type LogLevel = keyof typeof LogLevelIndex & string & NonNullable<unknown>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the & string & NonNullable<unknown> segment can be removed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, you are right, it looks like autocomplete with the keyof typeof work without shenanigans.

@tabaktoni tabaktoni requested a review from penovicp January 29, 2025 12:31
@tabaktoni tabaktoni merged commit d0ffbcc into develop Jan 29, 2025
3 checks passed
github-actions bot pushed a commit that referenced this pull request Jan 29, 2025
# [6.23.0](v6.22.0...v6.23.0) (2025-01-29)

### Features

* logger, config, deprecate legacy tx ([#1302](#1302)) ([d0ffbcc](d0ffbcc))
Copy link

🎉 This PR is included in version 6.23.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants