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

fix: Adding SDK support for channel tags APIs #1347

Merged
merged 2 commits into from
Jun 25, 2024
Merged

Conversation

Razaso
Copy link
Collaborator

@Razaso Razaso commented Jun 11, 2024

Fixes Issue

Changes proposed

Check List (Check all the applicable boxes)

  • My code follows the code style of this project.
  • My change requires changes to the documentation.
  • I have updated the documentation accordingly.
  • This PR does not contain plagiarized content.
  • The title of my pull request is a short description of the requested changes.

Screenshots

Note to reviewers

Copy link

  • In packages/restapi/src/lib/channels/getTags.ts:

    • There is a missing comment indicating the start of the return statement in the getTags function.
    • The indentation for the export type GetTagsOptionsType definition and the export const getTags function is inconsistent, it should be aligned properly.
    • Typo in the comment, address of the channel should be Address of the channel.
  • In packages/restapi/src/lib/config.ts:

    • There are syntax errors in the VIEM_CORE_CONFIG, CORE_CONFIG, and CONFIG objects due to missing closing curly braces and incorrect key references.
  • In packages/restapi/src/lib/pushNotification/channel.ts:

    • There are multiple syntax errors in the create and update methods related to missing closing parentheses, closing curly braces, and bracket mismatches.

Apart from the identified issues, the code seems to have logical errors like missing logic in methods, but this requires a deeper analysis of the functionality.

Overall, the code needs some fixes in syntax and typos.

Copy link

In the code review, I have identified the following issues:

File: packages/restapi/src/lib/channels/getTags.ts

  1. There is a missing comment closure for the comment line just before the export const getTags = async ( function.
  2. In the GetTagsOptionsType interface, the channel property is described as the address of the channel, but it should be updated to specify what type of address it is (e.g., CAIP address, Ethereum address).
  3. In the getTags function, the usage of optional chaining (?.) in response.data?.tags may cause a runtime error if data is null or undefined. It's wise to handle this scenario more gracefully.
  4. The error message logged in the catch block mentions [EPNS-SDK], but it seems related to the Push SDK. It should be updated to reflect the correct SDK.

File: packages/restapi/src/lib/config.ts

  1. In the VIEM_CORE_CONFIG object, there are missing closing braces for the ENV.STAGING and ENV.DEV configurations, leading to syntax errors.
  2. In the CORE_CONFIG object, the assignment after const CONFIG = { is incomplete. There seems to be a missing closing bracket and semicolon.

File: packages/restapi/src/lib/pushNotification/PushNotificationTypes.ts

  1. In the IAdvance interface, the comment before the ipfs property is incomplete.
  2. In the NotificationSetting interface, the ticker property is not defined. It should be added for clarity.
  3. In the ChannelFeedsOptions interface, NotifictaionType is likely a typo and should be corrected to NotificationType.

File: packages/restapi/src/lib/pushNotification/channel.ts

  1. In the update method of the Channel class, there are missing closing braces and semicolons at the end of the method. These should be added for correct syntax.
  2. In the create method, there is a missing closing parenthesis at the end of the const fees = viem.parseUnits( line.

After addressing these issues, the code should be in better shape. If you need any more assistance, feel free to ask.

All looks good.

@Razaso Razaso requested review from akp111, Aman035 and mohammeds1992 and removed request for akp111, Aman035 and mohammeds1992 June 11, 2024 13:49
@Razaso Razaso merged commit 4d4f484 into main Jun 25, 2024
1 check passed
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.

2 participants