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: add apis for initiate and verify alias. #1239

Merged
merged 2 commits into from
Apr 28, 2024

Conversation

Razaso
Copy link
Collaborator

@Razaso Razaso commented Apr 19, 2024

Fixes Issue

#1218
#1219

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

Review Feedback

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

  1. Typo in NotifictaionType, should be NotificationType.
  2. Typo in SubscribeUnsubscribeOptions, missing closing curly brace }.
  3. Typo in UserSetting, 'VIEM_CONFIG' should be 'VIEM CONFIG'.
  4. Typo in FeedsOptions, the comment mentions changing to string[] but it is already an array of strings, so the comment is unnecessary.
  5. Typo in INotification, should be INotification.
  6. Typo in IPayload, should be IPayload.
  7. Typo in IConfig, missing closing curly brace }.

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

  1. Typo in config import, VIEM_CONFIG should be VIEM CONFIG.
  2. Syntax error in the initiate method, missing closing curly brace }.
  3. Syntax error in the verify method, missing closing curly brace }.

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

  1. Typo in ProgressHookType, should be imported.
  2. Typo in LENGTH_LOWER_LIMIT, 'LIMTI' should be 'LIMIT'.

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

  1. Typo in LENGTH_UPPER_LIMIT, typo in variable name.
  2. Typo in SLIDER_TYPE and BOOLEAN_TYPE. Incorrect variable declaration.
  3. Typo in aliasInCaip, incorrect parameter spelling on comment.
  4. Typo in verifyAlias, missing closing curly brace }.
  5. Typo in getAliasInfo, issue with return type, not handling Axios response correctly.
  6. Typo in getAddressFromCaip, incorrect variable name 'caipAddress' should be 'caip'

Overall, there are several typos, syntax errors, and incorrect variable declarations across the files. Please review and make the necessary corrections.


I have identified various issues in the provided code snippets. Please make the necessary corrections. Let me know if you need any further assistance.

@Aman035
Copy link
Member

Aman035 commented Apr 23, 2024

1 issue which I can think of in this approach is
Lets suppose a channel on ETH ( EVM chain ) is creating alias on Solana ( Non EVM chain )
In this case the channel owner can never interact with comm contract on solana, thus they will never be able to call alias.initiate

@Razaso Razaso closed this Apr 23, 2024
@Razaso
Copy link
Collaborator Author

Razaso commented Apr 23, 2024

1 issue which I can think of in this approach is Lets suppose a channel on ETH ( EVM chain ) is creating alias on Solana ( Non EVM chain ) In this case the channel owner can never interact with comm contract on solana, thus they will never be able to call alias.initiate

You meant alias.verify? Because alias.initiate is something which is invoked to add an alias.
and also, do we support this as of today and needs to be fixed ?

@Razaso Razaso reopened this Apr 23, 2024
Copy link

There are several issues found in the provided code:

  1. In PushNotificationTypes.ts:

    • Typo in the NotifictaionType should be corrected to NotificationType.
    • Typo in the INBOX value inside the FeedType enum, should be INBOX = 'INBOX'.
  2. In PushNotificationTypes.ts:

    • Typo in the ChannelInfoOptions type, missing '}' at the end.
    • Typo in the SubscribeUnsubscribeOptions type, missing '}' at the end.
    • Typo in the UserSetting type, missing '}' at the end.
  3. In PushNotificationTypes.ts:

    • Typo in the FeedType enum, missing '}' at the end.
  4. In alias.ts:

    • Typo in the initiate method, missing '}' at the end.
    • Typo in the initiate method, the throw new Error statement is not in the catch block.
    • Typo in the verify method, missing '}' at the end.
    • Typo in the verify method, the throw new Error statement is not in the catch block.
  5. In channel.ts:

    • Typo in the search method, missing '}' at the end.
  6. In pushNotificationBase.ts:

    • Typo in the class import, Signer should be imported from the correct path.
    • Typo in the LENGTH_LOWER_LIMTI constant, should be LENGTH_LOWER_LIMIT.
    • Typo in the SETTING_SEPARATOR constant, should be '-' instead of '+'.
    • Typo in the DEFAULT_ENABLE_VALUE constant, should be '1'.
    • Typo in the DEFAULT_TICKER_VALUE constant, should be '1'.
    • Typo in the verifyAlias method, missing '}' at the end.

After correcting the above issues, the code looks good.

Note: Please check for any other logic-related issues or business rule validations as these were not thoroughly analyzed in the review.

Copy link

In the file packages/restapi/src/lib/pushNotification/PushNotificationTypes.ts:

  1. In the ChannelInfoOptions type, there is a missing closing curly brace '}' at the end.
  2. In the SubscribeUnsubscribeOptions type, there is a missing closing curly brace '}' at the end.
  3. In the UserSetting type, the 'value' property is defined as a number or an object with 'lower' and 'upper' properties. It should have a closing curly brace '}' after the 'upper' property definition.
  4. In the FeedType enum, the SPAM value is missing a closing single quote.
  5. In the FeedsOptions type, there is a comment 'TODO: change it to string[] once we start supporting multiple channel', which seems to be incomplete.
  6. In the ChannelSearchOptions type, there is a missing closing curly brace '}' after the comment "Types related to notification".

In the file packages/restapi/src/lib/pushNotification/alias.ts:

  1. In the initiate method, the try block is not closed before the comment "// @description adds an alias to the channel".
  2. In the initiate method, there is a missing closing curly brace '}' at the end after the catch block.
  3. In the verify method, there is a missing closing curly brace '}' at the end after the catch block.
  4. In the verify method, there are several ellipsis (...) at the end, without a proper block closing.
  5. In the verify method, there is a reference to 'error' variable in the catch block without being defined.
  6. In the verify method, there is missing error handling logic for the successful transaction case.

In the file packages/restapi/src/lib/pushNotification/channel.ts:

  1. In the search method, the comment for the description should be '@description - returns relevant information as per the query that was passed'.
  2. In the search method, there is a missing closing curly brace '}' after the return statement.

In the file packages/restapi/src/lib/pushNotification/pushNotificationBase.ts:

  1. In the initiateAddAlias method, there is an incomplete condition block where the 'throw new Error' statement is inside an IF block but the closing curly brace is missing.
  2. In the verifyAlias method, there is an incomplete condition block similar to the initiateAddAlias method.
  3. In the verifyAlias method, there are several ellipsis (...) at the end without a proper block closing.

Overall, the code has various syntax errors, missing curly braces, incomplete logic blocks, and unfinished comments. It needs to be corrected to maintain the code's integrity.

});

it('With signer and provider :: should add alias', async () => {
const res = await userKate.channel.alias.initiate(
Copy link
Member

Choose a reason for hiding this comment

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

Also currently verify fails since account does not have tokens on the chain
image

@Razaso Razaso merged commit 34859e4 into main Apr 28, 2024
2 checks 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