Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThis pull request introduces a new Jest configuration for TypeScript testing and updates the package configuration with new dependencies, peer dependencies, and export mappings. It adds multiple codec modules (for permissions, public key, and transactions) along with their error handling classes and enumerations. New messaging classes for handling sign-in variants (including relay sign-in, standard sign-in, initial transactions, and fake signatures) are introduced alongside wallet client interfaces. Additional test suites and global mocks have been provided, and minor cleanup (such as the removal of an empty ESLint rules object) has been performed. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MsgSignIn
participant WalletClient
Client->>MsgSignIn: Provide sign-in URL
MsgSignIn->>MsgSignIn: Parse URL (extract accountID & signingURL)
MsgSignIn->>WalletClient: Pass MsgSignIn object
WalletClient-->>Client: Process sign-in request
sequenceDiagram
participant Client
participant MsgRelaySignIn
participant RelayerWalletClient
Client->>MsgRelaySignIn: Provide relay sign-in URL
MsgRelaySignIn->>MsgRelaySignIn: Parse URL (extract accountID, signingURL, relayerAPI)
MsgRelaySignIn->>RelayerWalletClient: Pass MsgRelaySignIn object
RelayerWalletClient-->>Client: Process relay sign-in request
Possibly related PRs
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (45)
packages/shared/core/src/modules/ena/index.ts (1)
1-1: Consider using a more descriptive variable nameThe variable name 'a' doesn't provide any context about its purpose or usage. Using a more descriptive name would improve code readability and maintainability.
-export const a = 1; +export const EXAMPLE_CONSTANT = 1; // Replace with a meaningful name based on its purposepackages/shared/core/src/modules/common/codecs/errors/codec.error-codes.ts (1)
1-4: LGTM: Good error code patternUsing an enum for error codes is a good practice. It ensures type safety and makes error handling more maintainable across the codebase.
Consider adding documentation for each error code
Adding JSDoc comments for each error code would help developers understand when these errors occur.
export enum CodecErrorCodes { + /** + * Error occurred while decoding URL parameters + */ ERROR_DECODING_URL = "ERROR_DECODING_URL", + /** + * Error occurred while encoding URL parameters + */ ERROR_ENCODING_URL = "ERROR_ENCODING_URL", }packages/shared/core/test/mocks/public-key.mock.ts (1)
1-6: Mock implementation looks good but consider enhancing it.The
PublicKeyMockprovides a basic mock implementation for thePublicKeytype from near-api-js, which is essential for testing. The use of themockifyutility is appropriate here.However, the mock only implements the
enumproperty. Consider adding other essential properties or methods that might be used during testing to make the mock more complete.export const PublicKeyMock = mockify<PublicKey>({ enum: "mockEnum", + toString: () => "ed25519:mockPublicKey", + verify: () => true, });packages/shared/core/src/modules/common/codecs/errors/codec.error.ts (1)
3-7: Error class implementation is clean but could be enhanced.The
CodecErrorclass extendsErrorand provides a clear error message format. However, it doesn't store the type separately, which could be useful for error handling.Consider adding a property to store the codec type for easier programmatic access.
export class CodecError extends Error { + readonly type: CodecType; constructor(type: CodecType, message: string) { super(`${type}: ${message}`); + this.type = type; + this.name = "CodecError"; } }packages/shared/core/src/modules/common/msgs/errors/msg.error.ts (1)
6-14: MsgError implementation looks good but could be enhanced.The
MsgErrorclass correctly extends the Error class and properly handles the error code as a readonly property. However, consider enhancing error messages by mapping error codes to more descriptive messages instead of just passing the code as the message.export class MsgError extends Error { /** * Constructor for the MsgError class. * @param code The error code. */ constructor(readonly code: MsgErrorCodes) { - super(code); + super(getErrorMessage(code)); } } + /** + * Get descriptive error message from error code + * @param code The error code + * @returns Human-readable error message + */ + function getErrorMessage(code: MsgErrorCodes): string { + // Map error codes to descriptive messages + const errorMessages: Record<MsgErrorCodes, string> = { + INVALID_URL: 'The provided URL is invalid or missing required parameters' + // Add other error codes and messages as needed + }; + + return errorMessages[code] || `Unknown error: ${code}`; + }packages/shared/core/src/modules/common/clients/i-near-wallet.client.ts (1)
1-12: Interface design looks clean and focussedThe
INearWalletClientinterface is well-structured with clear JSDoc documentation. It properly imports theMsgSignInfrom the messages module and defines a single responsibility for wallet client implementations.Consider whether the
signInmethod should return a Promise for better handling of asynchronous operations. Since wallet sign-in typically involves network operations or redirects, a Promise return type might be more appropriate:- signIn(msg: MsgSignIn): void; + signIn(msg: MsgSignIn): Promise<void>;This would allow consumers to await the completion of the sign-in process or handle errors more effectively.
packages/shared/core/jest.config.ts (1)
1-16: Jest configuration is appropriately set up but has minimal coverage requirementsThe Jest configuration is well-structured for a TypeScript project, but the coverage thresholds are set very low (0 for both branches and statements). While this may be acceptable for early development, consider increasing these thresholds as the codebase grows to ensure proper test coverage.
coverageThreshold: { global: { - branches: 0, - statements: 0, + branches: 70, + statements: 80, }, },Additionally, you might want to include lines and functions in your coverage metrics:
coverageThreshold: { global: { branches: 70, statements: 80, + lines: 80, + functions: 90, }, },packages/shared/core/test/mocks/transaction.mock.ts (1)
1-12: Transaction mock implementation looks goodThe mock implementation for the NEAR Transaction type is well-structured, providing mock values for all required properties. The use of the
mockifyutility creates a flexible mock that can be extended in tests if needed.One suggestion: Consider adding a comment explaining the mock's purpose and perhaps a basic example of how to customize it in tests:
import { Transaction } from "near-api-js/lib/transaction"; import { mockify } from "@shared/test"; import { PublicKeyMock } from "./public-key.mock"; +/** + * Mock implementation of the NEAR Transaction object. + * Example usage with custom properties: + * ``` + * const customTransaction = new TransactionMock({ + * signerId: "custom-signer", + * receiverId: "custom-receiver" + * }); + * ``` + */ export const TransactionMock = mockify<Transaction>({ signerId: "mockSignerId", nonce: 1, actions: [], blockHash: "mockBlockHash", publicKey: new PublicKeyMock(), receiverId: "mockReceiverId", });packages/shared/core/test/mocks/transaction.codec.mock.ts (1)
5-16: TransactionCodecGlobalMock implementation could benefit from more documentationThe global mock for TransactionCodec is implemented correctly, but the base64-encoded transaction string is complex and its purpose is not immediately clear. Consider adding a comment explaining what this encoded transaction represents to improve maintainability.
export const TransactionCodecGlobalMock = createGlobalMock(TransactionCodec, { fromURLParam: new MethodMock( "mockReturnValue", + // This base64 string decodes to a mock transaction with the following properties: + // signerId: "mockSignerId", receiverId: "mockReceiverId", etc. Transaction.decode( Buffer.from( "DAAAAG1vY2tTaWduZXJJZACcdRJ+JQOaKfqxr8M0e4Ylfcz1E107wi1yLO+td5FwvAEAAAAAAAAADgAAAG1vY2tSZWNlaXZlcklko7RnCT5MjkhJMV2ASLYPwYZorzhqPrGdadCYH6KZGxQAAAAA", "base64", ), ), ), toURLParam: new MethodMock("mockReturnValue", "mockEncodedTransaction"), });Additionally, consider adding mock implementations that test error scenarios to ensure robust testing of error handling:
export const TransactionCodecGlobalMock = createGlobalMock(TransactionCodec, { fromURLParam: new MethodMock( "mockReturnValue", Transaction.decode( Buffer.from( "DAAAAG1vY2tTaWduZXJJZACcdRJ+JQOaKfqxr8M0e4Ylfcz1E107wi1yLO+td5FwvAEAAAAAAAAADgAAAG1vY2tSZWNlaXZlcklko7RnCT5MjkhJMV2ASLYPwYZorzhqPrGdadCYH6KZGxQAAAAA", "base64", ), ), ), toURLParam: new MethodMock("mockReturnValue", "mockEncodedTransaction"), + // Example of how to create an error-throwing mock for testing error scenarios: + // errorFromURLParam: new MethodMock( + // "mockImplementation", + // () => { + // throw new CodecError(CodecType.TRANSACTION_CODEC, CodecErrorCodes.ERROR_DECODING_URL); + // } + // ), });packages/shared/core/test/modules/common/msgs/fak-sign.msg.spec.ts (1)
1-63: Potential filename typoThe filename appears to be "fak-sign.msg.spec.ts" instead of "fake-sign.msg.spec.ts", which could cause confusion. Consider renaming the file to match the class naming convention.
packages/shared/core/test/modules/common/codecs/public-key.codec.spec.ts (1)
27-27: Consider testing with a typed null valueUsing
nullwithout type information might trigger TypeScript errors if strict null checks are enabled. Consider using explicit type casting:-expect(() => PublicKeyCodec.toURLParam(null)).toThrow(`${CodecType.PUBLIC_KEY_CODEC}: ${CodecErrorCodes.ERROR_ENCODING_URL}`); +expect(() => PublicKeyCodec.toURLParam(null as any)).toThrow(`${CodecType.PUBLIC_KEY_CODEC}: ${CodecErrorCodes.ERROR_ENCODING_URL}`);packages/shared/core/test/modules/common/codecs/permissions.codec.spec.ts (2)
1-29: Tests look good but descriptions could be more precise.The tests thoroughly verify both encoding and decoding functionality of the
PermissionsCodec. However, there's a minor issue with the test description.In the
toURLParamtest at line 22, the description says "should return a URL", but the method actually returns a base64-encoded string representation of the permissions object, not a URL. Consider updating the description to "should return a base64-encoded string" for better clarity.- describe("toURLParam", () => { - it("should return a URL", () => { + describe("toURLParam", () => { + it("should return a base64-encoded string", () => {
22-27: Consider adding a round-trip test.The current test directly compares against a hardcoded encoded string. For more robust testing, consider adding a round-trip test that encodes, then decodes, and compares with the original object.
it("should return a URL", () => { const url = PermissionsCodec.toURLParam(mockPermissions); expect(url).toEqual( "eyJmdW5jdGlvbkNhbGwiOnsicmVjZWl2ZXJJZCI6Im1vY2tSZWNlaXZlcklkIiwibWV0aG9kTmFtZSI6Im1vY2tNZXRob2ROYW1lIiwiYXJncyI6Im1vY2tBcmdzIn19", ); }); + it("should provide consistent encoding and decoding", () => { + const encoded = PermissionsCodec.toURLParam(mockPermissions); + const decoded = PermissionsCodec.fromURLParam(encoded); + expect(decoded).toEqual(mockPermissions); + });packages/shared/core/src/modules/relayer/msgs/relay-sign-in.msg.ts (1)
46-52: Minor consistency suggestion in property access.For consistency, consider using either direct property access or getter methods throughout the class. Currently, the
toURLmethod uses getters (this.accountID), while other parts of the class use direct property access (this._accountID).toURL(url: string): string { const urlObj = new URL(url); - urlObj.searchParams.set("accountID", this.accountID); - urlObj.searchParams.set("signingURL", this.signingURL); - urlObj.searchParams.set("relayerAPI", this.relayerAPI); + urlObj.searchParams.set("accountID", this._accountID); + urlObj.searchParams.set("signingURL", this._signingURL); + urlObj.searchParams.set("relayerAPI", this._relayerAPI); return urlObj.toString(); }Alternatively, you could use the getters consistently throughout the class, which is the more object-oriented approach.
packages/shared/core/test/modules/common/msgs/sign-in.msg.spec.ts (4)
6-11: Simplify nested test structure.There's unnecessary nesting in the test structure with duplicate descriptions. The outer and inner descriptions are identical, which adds redundancy without providing additional clarity.
- describe("should throw an error if accountID is not provided", () => { it("should throw an error if accountID is not provided", () => { const url = "https://example.com?signingURL=https://example.com"; expect(() => MsgSignIn.fromURL(url)).toThrow(MsgErrorCodes.INVALID_URL); }); - });
13-18: Simplify nested test structure.Similar to the previous comment, this test has duplicate descriptions in the describe and it blocks.
- describe("should throw an error if signingURL is not provided", () => { it("should throw an error if signingURL is not provided", () => { const url = "https://example.com?accountID=example.com"; expect(() => MsgSignIn.fromURL(url)).toThrow(MsgErrorCodes.INVALID_URL); }); - });
20-33: Simplify nested test structure.This test structure has the same issue with duplicate descriptions.
- describe("should create a new MsgSignIn object", () => { it("should create a new MsgSignIn object", () => { const accountID = "example.com"; const signingURL = "https://example.com"; const url = `https://example.com?accountID=${accountID}&signingURL=${signingURL}`; const msg = MsgSignIn.fromURL(url); expect(msg).toBeDefined(); expect(msg.accountID).toEqual(accountID); expect(msg.signingURL).toEqual(signingURL); }); - });
36-46: Simplify nested test structure.This test also has duplicate descriptions in the describe and it blocks.
- describe("should create a new MsgSignIn object", () => { it("should create a new MsgSignIn object", () => { const accountID = "example.com"; const signingURL = "https://example.com"; const encodedSigningURL = encodeURIComponent(signingURL); const msg = new MsgSignIn(accountID, signingURL); const url = msg.toURL("https://example.com"); expect(url).toEqual(`https://example.com/?accountID=${accountID}&signingURL=${encodedSigningURL}`); }); - });packages/shared/core/src/modules/common/codecs/transaction.codec.ts (3)
8-36: Consider using standalone functions instead of a static-only class.The
TransactionCodecclass currently consists of only static methods. Following modern JS/TS best practices, consider converting this to standalone functions for better maintainability.-export class TransactionCodec { - /** - * Decodes a transaction from a URL. - * @param transaction The transaction to decode. - * @returns The decoded transaction. - */ - static fromURLParam(transaction: string): Transaction { +/** + * Decodes a transaction from a URL. + * @param transaction The transaction to decode. + * @returns The decoded transaction. + */ +export function fromURLParam(transaction: string): Transaction { try { const encodedTransaction = Buffer.from(transaction, "base64"); return Transaction.decode(encodedTransaction); } catch (_: unknown) { throw new CodecError(CodecType.TRANSACTION_CODEC, CodecErrorCodes.ERROR_DECODING_URL); } - } +} - /** - * Encodes a transaction to a URL. - * @param transaction The transaction to encode. - * @returns The encoded transaction. - */ - static toURLParam(transaction: Transaction): string { +/** + * Encodes a transaction to a URL. + * @param transaction The transaction to encode. + * @returns The encoded transaction. + */ +export function toURLParam(transaction: Transaction): string { try { const encodedTransaction = Buffer.from(transaction.encode()).toString("base64"); return encodedTransaction; } catch (_: unknown) { throw new CodecError(CodecType.TRANSACTION_CODEC, CodecErrorCodes.ERROR_ENCODING_URL); } - } -} +}🧰 Tools
🪛 Biome (1.9.4)
[error] 8-36: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
18-20: Add more specific error messaging for debugging.The current error handling catches all errors but doesn't provide detailed information about what went wrong. Consider capturing the error message to aid in debugging.
} catch (_: unknown) { - throw new CodecError(CodecType.TRANSACTION_CODEC, CodecErrorCodes.ERROR_DECODING_URL); + throw new CodecError(CodecType.TRANSACTION_CODEC, CodecErrorCodes.ERROR_DECODING_URL); }
28-34: Consider adding input validation before encoding.The method attempts to encode the transaction without verifying if it's a valid Transaction object, which could lead to unclear errors.
static toURLParam(transaction: Transaction): string { try { + if (!transaction || typeof transaction.encode !== 'function') { + throw new Error('Invalid transaction object'); + } const encodedTransaction = Buffer.from(transaction.encode()).toString("base64"); return encodedTransaction; } catch (_: unknown) { throw new CodecError(CodecType.TRANSACTION_CODEC, CodecErrorCodes.ERROR_ENCODING_URL); } }packages/shared/core/test/modules/relayer/msgs/relay-sign-in.msg.spec.ts (5)
10-15: Remove redundant describe nesting.The describe and it statements contain the same message, which is redundant. Consider simplifying the test structure.
- describe("should throw an error if accountID is not provided", () => { it("should throw an error if accountID is not provided", () => { const url = `https://example.com?signingURL=${signingURL}&relayerAPI=${relayerAPI}`; expect(() => MsgRelaySignIn.fromURL(url)).toThrow(MsgErrorCodes.INVALID_URL); }); - });
17-22: Remove redundant describe nesting.Similar to the previous comment, the nested describe is redundant.
- describe("should throw an error if signingURL is not provided", () => { it("should throw an error if signingURL is not provided", () => { const url = `https://example.com?accountID=${accountID}&relayerAPI=${relayerAPI}`; expect(() => MsgRelaySignIn.fromURL(url)).toThrow(MsgErrorCodes.INVALID_URL); }); - });
24-29: Remove redundant describe nesting.Same issue with nested describe redundancy.
- describe("should throw an error if relayerAPI is not provided", () => { it("should throw an error if relayerAPI is not provided", () => { const url = `https://example.com?accountID=${accountID}&signingURL=${signingURL}`; expect(() => MsgRelaySignIn.fromURL(url)).toThrow(MsgErrorCodes.INVALID_URL); }); - });
31-41: Remove redundant describe and enhance test specificity.Consider removing redundant describe and enhancing test specificity by validating each property independently.
- describe("should create a new MsgRelaySignIn object", () => { it("should create a new MsgRelaySignIn object", () => { const url = `https://example.com?accountID=${accountID}&signingURL=${signingURL}&relayerAPI=${relayerAPI}`; const msg = MsgRelaySignIn.fromURL(url); - expect(msg).toBeDefined(); + // Verify the msg object type + expect(msg).toBeInstanceOf(MsgRelaySignIn); expect(msg.accountID).toEqual(accountID); expect(msg.signingURL).toEqual(signingURL); expect(msg.relayerAPI).toEqual(relayerAPI); }); - });
45-51: Add test for URL parameter encoding.The test verifies that the URL is constructed correctly, but doesn't explicitly validate the encoding of parameters containing special characters.
describe("toURL", () => { it("should return a URL", () => { const msg = new MsgRelaySignIn(accountID, signingURL, relayerAPI); const url = msg.toURL(relayerAPI); expect(url).toEqual( `${relayerAPI}/?accountID=${accountID}&signingURL=${encodeURIComponent(signingURL)}&relayerAPI=${encodeURIComponent(relayerAPI)}`, ); }); + + it("should properly encode URL parameters with special characters", () => { + const specialAccountID = "example/user@domain.com"; + const msg = new MsgRelaySignIn(specialAccountID, signingURL, relayerAPI); + const url = msg.toURL(relayerAPI); + expect(url).toContain(`accountID=${encodeURIComponent(specialAccountID)}`); + }); });packages/shared/core/test/modules/common/msgs/sign-initial-tx.msg.spec.ts (4)
25-30: Remove redundant describe nesting.The nested describe is redundant as it duplicates the it statement. Consider simplifying the test structure.
- describe("should throw an error if transaction is not provided", () => { it("should throw an error if transaction is not provided", () => { const url = "https://example.com"; expect(() => MsgSignInitialTx.fromURL(url)).toThrow(MsgErrorCodes.INVALID_URL); }); - });
32-38: Remove redundant describe nesting.Similar redundancy in the nested describe.
- describe("should throw an error if redirectURL is not provided", () => { it("should throw an error if redirectURL is not provided", () => { const urlTransaction = "mockEncodedTransaction"; const url = `https://example.com?transaction=${urlTransaction}`; expect(() => MsgSignInitialTx.fromURL(url)).toThrow(MsgErrorCodes.INVALID_URL); }); - });
40-47: Remove redundant describe nesting.Same redundancy issue with nested describe.
- describe("should throw an error if permissions are not provided", () => { it("should throw an error if permissions are not provided", () => { const urlTransaction = "mockEncodedTransaction"; const urlRedirectURL = "mockEncodedRedirectURL"; const url = `https://example.com?transaction=${urlTransaction}&redirectURL=${urlRedirectURL}`; expect(() => MsgSignInitialTx.fromURL(url)).toThrow(MsgErrorCodes.INVALID_URL); }); - });
49-57: Remove redundant describe nesting.Same issue with redundant nested describe.
- describe("should throw an error if publicKey is not provided", () => { it("should throw an error if publicKey is not provided", () => { const urlTransaction = "mockEncodedTransaction"; const urlRedirectURL = "mockEncodedRedirectURL"; const urlPermissions = "mockEncodedPermissions"; const url = `https://example.com?transaction=${urlTransaction}&redirectURL=${urlRedirectURL}&permissions=${urlPermissions}`; expect(() => MsgSignInitialTx.fromURL(url)).toThrow(MsgErrorCodes.INVALID_URL); }); - });packages/shared/core/test/modules/common/codecs/transaction.codec.spec.ts (1)
40-45: Add explicit test for encoded result format.The test verifies that the encoded result matches the original, but doesn't test the format of the encoded string. Consider adding a specific test for the base64 format.
it("should return a URL", () => { const encodedTransaction = "DAAAAG1vY2tTaWduZXJJZACcdRJ+JQOaKfqxr8M0e4Ylfcz1E107wi1yLO+td5FwvAEAAAAAAAAADgAAAG1vY2tSZWNlaXZlcklko7RnCT5MjkhJMV2ASLYPwYZorzhqPrGdadCYH6KZGxQAAAAA"; const url = TransactionCodec.toURLParam(TransactionCodec.fromURLParam(encodedTransaction)); expect(url).toEqual(encodedTransaction); + + // Verify the encoded string is valid base64 + expect(() => Buffer.from(url, 'base64')).not.toThrow(); });packages/shared/core/src/modules/common/msgs/sign-initial-tx.msg.ts (4)
37-37: Fix typo in variable nameThere's a typo in the variable name
encodedTrasaction- it should beencodedTransactionfor better code readability and consistency.- const encodedTrasaction = urlObj.searchParams.get("transaction"); + const encodedTransaction = urlObj.searchParams.get("transaction");Don't forget to update the variable reference on line 42 and 46 as well.
42-44: Update error handling to provide more specific error messagesThe current error handling throws a generic
INVALID_URLerror when any of the required parameters is missing. Consider providing more specific error information to help with debugging.- if (!encodedTrasaction || !redirectURL || !urlPermissions || !urlPK) { - throw new MsgError(MsgErrorCodes.INVALID_URL); - } + if (!encodedTrasaction) { + throw new MsgError(MsgErrorCodes.INVALID_URL, "Missing 'transaction' parameter"); + } + if (!redirectURL) { + throw new MsgError(MsgErrorCodes.INVALID_URL, "Missing 'redirectURL' parameter"); + } + if (!urlPermissions) { + throw new MsgError(MsgErrorCodes.INVALID_URL, "Missing 'permissions' parameter"); + } + if (!urlPK) { + throw new MsgError(MsgErrorCodes.INVALID_URL, "Missing 'publicKey' parameter"); + }Note: This assumes the MsgError constructor accepts a second parameter for a detailed message. If not, consider updating the error class to support detailed error messages.
53-57: Fix JSDoc comment for toURL methodThe JSDoc comment for the
toURLmethod incorrectly describes the parameter as "The URL to create the MsgSignInitialTx object from" which contradicts its actual purpose.- * Creates a URL from the MsgSignInitialTx object. - * @param url The URL to create the MsgSignInitialTx object from. + * Creates a URL from the MsgSignInitialTx object. + * @param url The base URL to append the MsgSignInitialTx parameters to.
58-66: Consider adding validation for the URL parameterThe
toURLmethod doesn't validate that the input URL is valid before attempting to use it. Consider adding validation to ensure the URL is properly formatted.toURL(url: string): string { + if (!url) { + throw new MsgError(MsgErrorCodes.INVALID_URL, "Base URL cannot be empty"); + } + + let urlObj: URL; + try { const urlObj = new URL(url); + } catch (error) { + throw new MsgError(MsgErrorCodes.INVALID_URL, "Invalid base URL format"); + } urlObj.searchParams.set("transaction", TransactionCodec.toURLParam(this._transaction)); urlObj.searchParams.set("redirectURL", this._redirectURL); urlObj.searchParams.set("permissions", PermissionsCodec.toURLParam(this._permissions)); urlObj.searchParams.set("publicKey", PublicKeyCodec.toURLParam(this._publicKey)); return urlObj.toString(); }packages/shared/core/src/modules/common/codecs/permissions.codec.ts (2)
7-33: Consider using simple functions instead of a static-only classThe
PermissionsCodecclass contains only static methods. Following modern JavaScript/TypeScript best practices, consider refactoring this into simple exported functions rather than a class with static methods.-export class PermissionsCodec { - /** - * Decodes a permissions object from a URL. - * @param permissions The permissions object to decode. - * @returns The decoded permissions object. - */ - static fromURLParam(permissions: string): object { - try { - return JSON.parse(Buffer.from(permissions, "base64").toString("utf-8")); - } catch (_: unknown) { - throw new CodecError(CodecType.PERMISSIONS_CODEC, CodecErrorCodes.ERROR_DECODING_URL); - } - } - - /** - * Encodes a permissions object to a URL. - * @param permissions The permissions object to encode. - * @returns The encoded permissions object. - */ - static toURLParam(permissions: object): string { - try { - return Buffer.from(JSON.stringify(permissions)).toString("base64"); - } catch (_: unknown) { - throw new CodecError(CodecType.PERMISSIONS_CODEC, CodecErrorCodes.ERROR_ENCODING_URL); - } - } -} +/** + * Decodes a permissions object from a URL. + * @param permissions The permissions object to decode. + * @returns The decoded permissions object. + */ +export function decodePermissionsFromURL(permissions: string): object { + try { + return JSON.parse(Buffer.from(permissions, "base64").toString("utf-8")); + } catch (_: unknown) { + throw new CodecError(CodecType.PERMISSIONS_CODEC, CodecErrorCodes.ERROR_DECODING_URL); + } +} + +/** + * Encodes a permissions object to a URL. + * @param permissions The permissions object to encode. + * @returns The encoded permissions object. + */ +export function encodePermissionsForURL(permissions: object): string { + try { + return Buffer.from(JSON.stringify(permissions)).toString("base64"); + } catch (_: unknown) { + throw new CodecError(CodecType.PERMISSIONS_CODEC, CodecErrorCodes.ERROR_ENCODING_URL); + } +}Note: This change would require updating imports and usage across the codebase.
🧰 Tools
🪛 Biome (1.9.4)
[error] 7-33: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
13-19: Consider adding type safety for permissions objectThe
permissionsparameter is typed as a genericobject, which doesn't provide type safety for the specific structure of permissions. Consider defining a more specific interface or type for the permissions object.+/** + * Interface representing the structure of permissions. + */ +export interface Permissions { + // Add specific permission properties here + // For example: + // canAccessAccount?: boolean; + // canTransferTokens?: boolean; + [key: string]: any; // Fallback for unknown properties +} + /** * Decodes a permissions object from a URL. * @param permissions The permissions object to decode. * @returns The decoded permissions object. */ -static fromURLParam(permissions: string): object { +static fromURLParam(permissions: string): Permissions { try { - return JSON.parse(Buffer.from(permissions, "base64").toString("utf-8")); + return JSON.parse(Buffer.from(permissions, "base64").toString("utf-8")) as Permissions; } catch (_: unknown) { throw new CodecError(CodecType.PERMISSIONS_CODEC, CodecErrorCodes.ERROR_DECODING_URL); } }Also update the
toURLParammethod similarly:-static toURLParam(permissions: object): string { +static toURLParam(permissions: Permissions): string {packages/shared/core/src/modules/common/codecs/public-key.codec.ts (2)
8-34: Consider using simple functions instead of a static-only classThe
PublicKeyCodecclass contains only static methods. Following modern JavaScript/TypeScript best practices, consider refactoring this into simple exported functions rather than a class with static methods.-export class PublicKeyCodec { - /** - * Decodes a public key from a URL. - * @param publicKey The public key to decode. - * @returns The decoded public key. - */ - static fromURLParam(publicKey: string): PublicKey { - try { - return PublicKey.fromString(publicKey); - } catch (_: unknown) { - throw new CodecError(CodecType.PUBLIC_KEY_CODEC, CodecErrorCodes.ERROR_DECODING_URL); - } - } - - /** - * Encodes a public key to a URL. - * @param publicKey The public key to encode. - * @returns The encoded public key. - */ - static toURLParam(publicKey: PublicKey): string { - try { - return publicKey.toString(); - } catch (_: unknown) { - throw new CodecError(CodecType.PUBLIC_KEY_CODEC, CodecErrorCodes.ERROR_ENCODING_URL); - } - } -} +/** + * Decodes a public key from a URL. + * @param publicKey The public key to decode. + * @returns The decoded public key. + */ +export function decodePublicKeyFromURL(publicKey: string): PublicKey { + try { + return PublicKey.fromString(publicKey); + } catch (_: unknown) { + throw new CodecError(CodecType.PUBLIC_KEY_CODEC, CodecErrorCodes.ERROR_DECODING_URL); + } +} + +/** + * Encodes a public key to a URL. + * @param publicKey The public key to encode. + * @returns The encoded public key. + */ +export function encodePublicKeyForURL(publicKey: PublicKey): string { + try { + return publicKey.toString(); + } catch (_: unknown) { + throw new CodecError(CodecType.PUBLIC_KEY_CODEC, CodecErrorCodes.ERROR_ENCODING_URL); + } +}Note: This change would require updating imports and usage across the codebase.
🧰 Tools
🪛 Biome (1.9.4)
[error] 8-34: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
14-20: Consider adding validation for the input stringThe
fromURLParammethod doesn't validate if the input string is empty or malformed before attempting to parse it. Consider adding basic validation to provide more specific error messages.static fromURLParam(publicKey: string): PublicKey { + if (!publicKey || publicKey.trim() === '') { + throw new CodecError(CodecType.PUBLIC_KEY_CODEC, CodecErrorCodes.INVALID_INPUT); + } + try { return PublicKey.fromString(publicKey); } catch (_: unknown) { throw new CodecError(CodecType.PUBLIC_KEY_CODEC, CodecErrorCodes.ERROR_DECODING_URL); } }Note: This assumes
INVALID_INPUTerror code exists or would be added to theCodecErrorCodesenum.packages/shared/core/src/modules/common/msgs/sign-in.msg.ts (3)
11-13: Consider making properties explicitly privateThe properties
_accountIDand_signingURLare prefixed with underscores indicating they should be private, but they are not explicitly marked asprivate. This could unintentionally allow external code to access these properties directly, bypassing the getter methods.export class MsgSignIn { - _accountID: string; - _signingURL: string; + private _accountID: string; + private _signingURL: string;
47-47: Improve method documentation for toURL parameterThe documentation for the
urlparameter in thetoURLmethod doesn't accurately describe its purpose. It currently says "The URL to create the MsgSignIn object from" which is confusing since this method is converting a MsgSignIn object to a URL, not the other way around./** * Creates a URL from the MsgSignIn object. - * @param url The URL to create the MsgSignIn object from. + * @param url The base URL to append the MsgSignIn parameters to. * @returns A URL. */
30-37: Enhance error handling in fromURL methodWhile the method correctly checks if parameters are missing, it doesn't validate the URL structure or handle potential errors from the URL constructor. If an invalid URL is provided, it will throw a generic error rather than a more specific
MsgError.Consider wrapping the URL parsing in a try-catch block to handle invalid URLs more gracefully:
static fromURL(url: string): MsgSignIn { + try { const urlObj = new URL(url); const accountID = urlObj.searchParams.get("accountID"); const signingURL = urlObj.searchParams.get("signingURL"); if (!accountID || !signingURL) { throw new MsgError(MsgErrorCodes.INVALID_URL); } return new MsgSignIn(accountID, signingURL); + } catch (error) { + if (error instanceof MsgError) { + throw error; + } + throw new MsgError(MsgErrorCodes.INVALID_URL); + } }packages/shared/core/src/modules/common/msgs/fak-sign.msg.ts (3)
8-8: Clarify the purpose of MsgFakSign classThe class name
MsgFakSignis not immediately clear about its purpose. Based on the code, it appears to handle transaction signing, but the "Fak" prefix is ambiguous. Consider adding more detailed documentation explaining what "FakSign" means in this context and when this class should be used versus other signing methods./** * Message for signing a transaction. + * This class represents a "fake sign" message, which is used for [explain purpose here, e.g., + * testing, simulating signatures without actual cryptographic operations, etc.]. + * + * @example + * ```typescript + * const transaction = new Transaction(...); + * const msg = new MsgFakSign(transaction, "https://redirect.example.com"); + * const url = msg.toURL("https://base.example.com"); + * ``` */
9-10: Consider making properties explicitly privateThe properties
_transactionand_redirectURLare prefixed with underscores indicating they should be private, but they are not explicitly marked asprivate. This could unintentionally allow external code to access these properties directly, bypassing the getter methods.export class MsgFakSign { - _transaction: Transaction; - _redirectURL: string; + private _transaction: Transaction; + private _redirectURL: string;
45-45: Improve method documentation for toURL parameterThe documentation for the
urlparameter in thetoURLmethod doesn't accurately describe its purpose. It currently says "The URL to create the MsgFakSign object from" which is confusing since this method is converting a MsgFakSign object to a URL, not the other way around./** * Creates a URL from the MsgFakSign object. - * @param url The URL to create the MsgFakSign object from. + * @param url The base URL to append the MsgFakSign parameters to. * @returns A URL. */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (39)
packages/shared/core/jest.config.ts(1 hunks)packages/shared/core/package.json(2 hunks)packages/shared/core/src/modules/common/clients/i-near-dapp.client.ts(1 hunks)packages/shared/core/src/modules/common/clients/i-near-wallet.client.ts(1 hunks)packages/shared/core/src/modules/common/clients/index.ts(1 hunks)packages/shared/core/src/modules/common/codecs/errors/codec.error-codes.ts(1 hunks)packages/shared/core/src/modules/common/codecs/errors/codec.error.ts(1 hunks)packages/shared/core/src/modules/common/codecs/errors/index.ts(1 hunks)packages/shared/core/src/modules/common/codecs/index.ts(1 hunks)packages/shared/core/src/modules/common/codecs/permissions.codec.ts(1 hunks)packages/shared/core/src/modules/common/codecs/public-key.codec.ts(1 hunks)packages/shared/core/src/modules/common/codecs/transaction.codec.ts(1 hunks)packages/shared/core/src/modules/common/codecs/types.ts(1 hunks)packages/shared/core/src/modules/common/msgs/errors/error-codes.ts(1 hunks)packages/shared/core/src/modules/common/msgs/errors/index.ts(1 hunks)packages/shared/core/src/modules/common/msgs/errors/msg.error.ts(1 hunks)packages/shared/core/src/modules/common/msgs/fak-sign.msg.ts(1 hunks)packages/shared/core/src/modules/common/msgs/index.ts(1 hunks)packages/shared/core/src/modules/common/msgs/sign-in.msg.ts(1 hunks)packages/shared/core/src/modules/common/msgs/sign-initial-tx.msg.ts(1 hunks)packages/shared/core/src/modules/ena/index.ts(1 hunks)packages/shared/core/src/modules/relayer/clients/i-near-relayer-wallet.client.ts(1 hunks)packages/shared/core/src/modules/relayer/clients/index.ts(1 hunks)packages/shared/core/src/modules/relayer/msgs/index.ts(1 hunks)packages/shared/core/src/modules/relayer/msgs/relay-sign-in.msg.ts(1 hunks)packages/shared/core/test/mocks/index.ts(1 hunks)packages/shared/core/test/mocks/permissions.codec.global-mock.ts(1 hunks)packages/shared/core/test/mocks/public-key.codec.global-mock.ts(1 hunks)packages/shared/core/test/mocks/public-key.mock.ts(1 hunks)packages/shared/core/test/mocks/transaction.codec.mock.ts(1 hunks)packages/shared/core/test/mocks/transaction.mock.ts(1 hunks)packages/shared/core/test/modules/common/codecs/permissions.codec.spec.ts(1 hunks)packages/shared/core/test/modules/common/codecs/public-key.codec.spec.ts(1 hunks)packages/shared/core/test/modules/common/codecs/transaction.codec.spec.ts(1 hunks)packages/shared/core/test/modules/common/msgs/fak-sign.msg.spec.ts(1 hunks)packages/shared/core/test/modules/common/msgs/sign-in.msg.spec.ts(1 hunks)packages/shared/core/test/modules/common/msgs/sign-initial-tx.msg.spec.ts(1 hunks)packages/shared/core/test/modules/relayer/msgs/relay-sign-in.msg.spec.ts(1 hunks)packages/shared/eslint/.eslintrc.package.js(0 hunks)
💤 Files with no reviewable changes (1)
- packages/shared/eslint/.eslintrc.package.js
🧰 Additional context used
🧬 Code Graph Analysis (22)
packages/shared/core/test/mocks/permissions.codec.global-mock.ts (3)
packages/shared/test/src/create-global-mock.ts (1)
createGlobalMock(20-43)packages/shared/core/src/modules/common/codecs/permissions.codec.ts (1)
PermissionsCodec(7-33)packages/shared/test/src/method-mock.ts (1)
MethodMock(9-17)
packages/shared/core/test/modules/common/msgs/fak-sign.msg.spec.ts (3)
packages/shared/core/test/mocks/transaction.codec.mock.ts (1)
TransactionCodecGlobalMock(5-16)packages/shared/core/test/mocks/transaction.mock.ts (1)
TransactionMock(5-12)packages/shared/core/src/modules/common/msgs/fak-sign.msg.ts (1)
MsgFakSign(8-68)
packages/shared/core/test/modules/common/codecs/public-key.codec.spec.ts (2)
packages/shared/core/src/modules/common/codecs/public-key.codec.ts (1)
PublicKeyCodec(8-34)packages/shared/core/src/modules/common/msgs/sign-initial-tx.msg.ts (1)
publicKey(96-98)
packages/shared/core/test/mocks/public-key.mock.ts (1)
packages/shared/test/src/mockify.ts (1)
mockify(12-18)
packages/shared/core/src/modules/common/clients/i-near-dapp.client.ts (2)
packages/shared/core/src/modules/common/msgs/sign-in.msg.ts (1)
MsgSignIn(11-69)packages/shared/core/src/modules/common/msgs/fak-sign.msg.ts (1)
MsgFakSign(8-68)
packages/shared/core/src/modules/common/clients/i-near-wallet.client.ts (1)
packages/shared/core/src/modules/common/msgs/sign-in.msg.ts (1)
MsgSignIn(11-69)
packages/shared/core/src/modules/common/codecs/transaction.codec.ts (3)
packages/shared/core/src/modules/common/msgs/fak-sign.msg.ts (1)
transaction(57-59)packages/shared/core/src/modules/common/msgs/sign-initial-tx.msg.ts (1)
transaction(72-74)packages/shared/core/src/modules/common/codecs/errors/codec.error.ts (1)
CodecError(3-7)
packages/shared/core/test/modules/common/codecs/transaction.codec.spec.ts (1)
packages/shared/core/src/modules/common/codecs/transaction.codec.ts (1)
TransactionCodec(8-36)
packages/shared/core/test/mocks/transaction.mock.ts (2)
packages/shared/test/src/mockify.ts (1)
mockify(12-18)packages/shared/core/test/mocks/public-key.mock.ts (1)
PublicKeyMock(4-6)
packages/shared/core/test/mocks/transaction.codec.mock.ts (3)
packages/shared/test/src/create-global-mock.ts (1)
createGlobalMock(20-43)packages/shared/core/src/modules/common/codecs/transaction.codec.ts (1)
TransactionCodec(8-36)packages/shared/test/src/method-mock.ts (1)
MethodMock(9-17)
packages/shared/core/src/modules/relayer/msgs/relay-sign-in.msg.ts (2)
packages/shared/core/src/modules/common/msgs/sign-in.msg.ts (2)
accountID(66-68)signingURL(58-60)packages/shared/core/src/modules/common/msgs/errors/msg.error.ts (1)
MsgError(6-14)
packages/shared/core/test/modules/common/codecs/permissions.codec.spec.ts (1)
packages/shared/core/src/modules/common/codecs/permissions.codec.ts (1)
PermissionsCodec(7-33)
packages/shared/core/test/modules/common/msgs/sign-initial-tx.msg.spec.ts (5)
packages/shared/core/test/mocks/transaction.codec.mock.ts (1)
TransactionCodecGlobalMock(5-16)packages/shared/core/test/mocks/permissions.codec.global-mock.ts (1)
PermissionsCodecGlobalMock(4-7)packages/shared/core/test/mocks/public-key.codec.global-mock.ts (1)
PublicKeyCodecGlobalMock(5-8)packages/shared/core/test/mocks/transaction.mock.ts (1)
TransactionMock(5-12)packages/shared/core/src/modules/common/msgs/sign-initial-tx.msg.ts (1)
MsgSignInitialTx(9-99)
packages/shared/core/test/mocks/public-key.codec.global-mock.ts (3)
packages/shared/test/src/create-global-mock.ts (1)
createGlobalMock(20-43)packages/shared/core/src/modules/common/codecs/public-key.codec.ts (1)
PublicKeyCodec(8-34)packages/shared/test/src/method-mock.ts (1)
MethodMock(9-17)
packages/shared/core/src/modules/common/codecs/public-key.codec.ts (2)
packages/shared/core/src/modules/common/msgs/sign-initial-tx.msg.ts (1)
publicKey(96-98)packages/shared/core/src/modules/common/codecs/errors/codec.error.ts (1)
CodecError(3-7)
packages/shared/core/src/modules/common/codecs/permissions.codec.ts (2)
packages/shared/core/src/modules/common/msgs/sign-initial-tx.msg.ts (1)
permissions(88-90)packages/shared/core/src/modules/common/codecs/errors/codec.error.ts (1)
CodecError(3-7)
packages/shared/core/src/modules/relayer/clients/i-near-relayer-wallet.client.ts (2)
packages/shared/core/src/modules/common/clients/i-near-wallet.client.ts (1)
INearWalletClient(6-12)packages/shared/core/src/modules/relayer/msgs/relay-sign-in.msg.ts (1)
MsgRelaySignIn(6-77)
packages/shared/core/test/modules/relayer/msgs/relay-sign-in.msg.spec.ts (2)
packages/shared/core/src/modules/common/msgs/sign-in.msg.ts (2)
signingURL(58-60)accountID(66-68)packages/shared/core/src/modules/relayer/msgs/relay-sign-in.msg.ts (4)
signingURL(66-68)relayerAPI(74-76)accountID(58-60)MsgRelaySignIn(6-77)
packages/shared/core/src/modules/common/msgs/sign-in.msg.ts (2)
packages/shared/core/src/modules/relayer/msgs/relay-sign-in.msg.ts (2)
accountID(58-60)signingURL(66-68)packages/shared/core/src/modules/common/msgs/errors/msg.error.ts (1)
MsgError(6-14)
packages/shared/core/test/modules/common/msgs/sign-in.msg.spec.ts (1)
packages/shared/core/src/modules/common/msgs/sign-in.msg.ts (3)
MsgSignIn(11-69)accountID(66-68)signingURL(58-60)
packages/shared/core/src/modules/common/msgs/fak-sign.msg.ts (3)
packages/shared/core/src/modules/common/msgs/sign-initial-tx.msg.ts (2)
transaction(72-74)redirectURL(80-82)packages/shared/core/src/modules/common/msgs/errors/msg.error.ts (1)
MsgError(6-14)packages/shared/core/src/modules/common/codecs/transaction.codec.ts (1)
TransactionCodec(8-36)
packages/shared/core/src/modules/common/msgs/sign-initial-tx.msg.ts (5)
packages/shared/core/src/modules/common/msgs/fak-sign.msg.ts (2)
transaction(57-59)redirectURL(65-67)packages/shared/core/src/modules/common/msgs/errors/msg.error.ts (1)
MsgError(6-14)packages/shared/core/src/modules/common/codecs/transaction.codec.ts (1)
TransactionCodec(8-36)packages/shared/core/src/modules/common/codecs/permissions.codec.ts (1)
PermissionsCodec(7-33)packages/shared/core/src/modules/common/codecs/public-key.codec.ts (1)
PublicKeyCodec(8-34)
🪛 Biome (1.9.4)
packages/shared/core/src/modules/common/codecs/transaction.codec.ts
[error] 8-36: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
packages/shared/core/src/modules/common/codecs/public-key.codec.ts
[error] 8-34: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
packages/shared/core/src/modules/common/codecs/permissions.codec.ts
[error] 7-33: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-base / Build
🔇 Additional comments (25)
packages/shared/core/test/mocks/permissions.codec.global-mock.ts (1)
1-7: LGTM: Good implementation of the permissions codec mockThe mock implementation correctly uses the createGlobalMock utility to create a test double for the PermissionsCodec class, with appropriate return values for both methods.
packages/shared/core/src/modules/relayer/msgs/index.ts (1)
1-1: LGTM: Proper export patternThis follows the standard pattern for re-exporting modules, which helps maintain a clean public API.
packages/shared/core/src/modules/relayer/clients/index.ts (1)
1-1: Clean and straightforward export, LGTM!This export pattern effectively makes the
INearRelayerWalletClientinterface available from the root module, aligning with the PR objective of defining new client interfaces.packages/shared/core/src/modules/common/msgs/errors/error-codes.ts (1)
1-3: Well-structured error code enumClean implementation of the error codes enumeration. This provides a good foundation for the messaging error handling system and follows TypeScript best practices.
packages/shared/core/src/modules/common/codecs/types.ts (1)
1-5: Well-defined codec type enumerationThis enum provides a clean, type-safe way to reference the three codec types mentioned in the PR objectives. The string literal values will make debugging easier when working with codec errors.
packages/shared/core/src/modules/common/msgs/errors/index.ts (1)
1-2: Appropriate exports structureThis index file effectively consolidates the error-related exports, following the standard TypeScript pattern for organizing module exports. This approach will make imports cleaner for consumers.
packages/shared/core/src/modules/common/clients/index.ts (1)
1-2: Exports follow standard pattern for modular organization.These exports follow a good practice by exposing the NEAR client interfaces through a central index file, making them accessible via a single import path. This promotes clean modular organization and helps maintain the API surface.
packages/shared/core/src/modules/common/msgs/index.ts (1)
1-3: Exports for message modules follow good organization patterns.These exports consolidate access to the various message types, following a clean modular structure. This approach makes it easy to import all message types from a single location.
packages/shared/core/src/modules/common/codecs/index.ts (1)
1-3: Clean exports structure for codec modules.The barrel file correctly exports all entities from the three codec modules, providing a clean, centralized import point that aligns with TypeScript best practices.
packages/shared/core/test/mocks/index.ts (1)
1-3: Well-organized mock exports for testing.The exports follow good testing practices by centralizing access to mock implementations, making them easier to import and use consistently across test files.
packages/shared/core/src/modules/common/codecs/errors/index.ts (1)
1-2: Proper error module exports structure.This barrel file correctly exports all entities from the codec error modules, following the same pattern as other index files in the codebase for consistency.
packages/shared/core/src/modules/relayer/clients/i-near-relayer-wallet.client.ts (1)
1-13: Clean interface design with clear documentationThe interface is well-designed as an extension of the base
INearWalletClientinterface, adding the relayer-specific functionality. The method signature and documentation are clear and follow good TypeScript practices.packages/shared/core/test/mocks/public-key.codec.global-mock.ts (1)
1-8: Well-structured mock implementationThe mock correctly implements the static methods from the original
PublicKeyCodecclass. The use ofcreateGlobalMockis appropriate for mocking static methods, and the hardcoded return values will facilitate consistent test behavior.packages/shared/core/test/modules/common/msgs/fak-sign.msg.spec.ts (4)
6-16: Well-organized test setupGood job setting up the test environment with appropriate mocks and test data. The use of
beforeEachto clear mocks ensures test isolation.
17-38: Comprehensive tests forfromURLmethodThe tests thoroughly cover the successful case and error cases for the
fromURLmethod, ensuring that it correctly handles URL parameters and throws appropriate errors when required parameters are missing.
40-48: Test case fortoURLmethodGood test for the
toURLmethod, verifying that it creates a URL properly and calls the expected codec method.
50-62: Getter method tests are completeThe tests for getter methods are simple and effective, ensuring they return the expected values.
packages/shared/core/test/modules/common/codecs/public-key.codec.spec.ts (2)
6-23: Thorough error handling tests forfromURLParamGood job testing both invalid inputs and the successful case for the
fromURLParammethod. The tests verify proper error messages and correctly check the returned value.
25-35: Well-structured tests fortoURLParamThe tests cover both the error case and successful operation of the method, ensuring that the encoding process works correctly.
packages/shared/core/src/modules/common/clients/i-near-dapp.client.ts (1)
1-18: Interface definition looks good!The
INearDAppClientinterface is well-structured with clear method signatures and appropriate documentation. The methodssignInitialTxandsignWithFakhave clear purposes and are well-documented.One minor suggestion: Consider clarifying what "Fak" stands for in the method name
signWithFakto improve code readability. If it's an abbreviation, adding a note in the documentation would help developers understand its purpose better.packages/shared/core/src/modules/relayer/msgs/relay-sign-in.msg.ts (1)
1-77: Implementation looks solid!The
MsgRelaySignInclass is well-structured with appropriate error handling and consistent with other message classes in the codebase.packages/shared/core/test/modules/common/msgs/sign-in.msg.spec.ts (1)
1-62: Overall test coverage is excellent.The tests provide comprehensive coverage of the
MsgSignInclass functionality, including error cases and getter methods. Despite the structural issues noted, the actual test logic is sound and effectively validates the class behavior.packages/shared/core/package.json (3)
24-25: Good practice: Adding peer dependencyAdding near-api-js as a peer dependency is a good practice as it clearly indicates which version of the library your package is compatible with.
21-21:Details
✅ Verification successful
Verify Near API version compatibility
The specified version "^5.1.1" for near-api-js seems unusual as the current stable versions are in the 1.x range. Double-check this version number to ensure it's correct and compatible with your project.
🌐 Web query:
What is the latest stable version of near-api-js?💡 Result:
The latest stable version of
near-api-jsis 5.1.1, released on March 20, 2025[7]. This version includes updates across its sub-packages like@near-js/wallet-account@1.3.3and@near-js/transactions@1.3.3, with bug fixes such as corrected transaction signing logic[1][7].Key improvements in recent releases:
- Transaction handling: Fixed hashing during transaction signing to ensure proper validation[1]
- Nonce management: Addressed BigInt compatibility issues when explicitly providing nonces[1]
- Dependency updates: Backend stability enhancements via dependencies like
exponential-backoff@3.1.2[1]The library remains actively maintained, with recent documentation updates and compatibility for NEAR Protocol's JSON-RPC interface[4][9].
Citations:
- 1: https://github.com/near/near-api-js/releases
- 2: https://www.npmjs.com/package/near-api-js/v/0.40.0
- 3: https://www.leewayhertz.com/create-smart-contracts-on-near-protocol/
- 4: https://github.com/near/near-api-js
- 5: https://pkg.go.dev/github.com/aurora-is-near/near-api-go
- 6: https://www.quicknode.com/docs/near
- 7: https://www.npmjs.com/package/near-api-js
- 8: https://chainstack.com/the-near-tutorial-set-quick-start-with-js/
- 9: https://docs.near.org/api/rpc/setup
- 10: https://docs.near.org/tools/near-api
Validated Near API version compatibility
The specified version
"^5.1.1"fornear-api-jsin thepackages/shared/core/package.jsonfile is verified as the latest stable release (released March 20, 2025). No changes are required.
3-3:Details
❓ Verification inconclusive
Verify package version reset
The package version has been changed from "1.0.0" to "0.0.0". This is unconventional and suggests a reset or development phase. Confirm that this change is intentional and aligns with your versioning strategy.
🌐 Web query:
What does a version "0.0.0" typically indicate in package.json?💡 Result:
A version
"0.0.0"inpackage.jsontypically indicates one of the following scenarios:1. Initial Development Phase
- Projects in pre-1.0.0 "initial development" use major version
0to signal instability. However,0.0.0is not part of standard semantic versioning (which starts at0.1.0) and often acts as a placeholder for:
- Unreleased or experimental code[4][8].
- Projects where the API is not yet stable and subject to frequent changes[4][8].
2. Automated Version Management
- Tools like
semantic-releasemay use0.0.0as a dummy value to avoid manual version bumps inpackage.json, relying instead on Git tags to track versions[2][6]. This reduces noise in version-control history for teams using automated publishing workflows[2][6].3. Default Tooling Behavior
- Older npm versions defaulted new projects to
0.0.0, though modern npm now uses1.0.0[8]. Legacy configurations or templates might still output0.0.0[6][8].Key Implications
- Not semver-compliant: The version
0.0.0isn’t recognized in semantic versioning rules, which start at0.1.0[4][10].- Registry compatibility: npm and other registries may reject
0.0.0during publishing, requiring a valid semver version (e.g.,0.0.1)[1][4].For clarity, most projects should start at
0.1.0or1.0.0unless intentionally using0.0.0for tooling-specific workflows[1][8].Citations:
- 1: https://docs.npmjs.com/about-semantic-versioning/
- 2: https://semantic-release.gitbook.io/semantic-release/support/faq
- 3: https://classic.yarnpkg.com/lang/en/docs/dependency-versions/
- 4: https://semver.org
- 5: https://spfx-app.dev/understanding-packagejson-versioning
- 6: aws/aws-cdk#7030
- 7: https://elliott-king.github.io/2020/08/package_json_symbols/
- 8: http://nodesource.com/blog/semver-tilde-and-caret/
- 9: https://gunnariauvinen.com/posts/what-do-the-tilde-carrot-and-asterick-mean-in-package-dot-json/
- 10: https://gist.github.com/jonlabelle/706b28d50ba75bf81d40782aa3c84b3e
Attention: Confirm the use of version "0.0.0"
The package version was changed from "1.0.0" to "0.0.0". From our research, a "0.0.0" version typically acts as a placeholder—indicating either an initial development phase, automated version management, or legacy tooling behavior—and is not semver-compliant. Please verify that this change is intentional and that it fits your versioning strategy and release workflow.
| describe("get transaction", () => { | ||
| it("should return the transaction", () => { | ||
| const msg = new MsgSignInitialTx(mockTransaction, mockRedirectURL, mockPermissions, mockPublicKey); | ||
| expect(msg.transaction).toEqual(mockTransaction); | ||
| }); | ||
| }); | ||
|
|
||
| describe("get redirectURL", () => { | ||
| it("should return the redirectURL", () => { | ||
| const msg = new MsgSignInitialTx(mockTransaction, mockRedirectURL, mockPermissions, mockPublicKey); | ||
| expect(msg.redirectURL).toEqual(mockRedirectURL); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing tests for permissions and publicKey getters.
The test suite is missing tests for the permissions and publicKey getter methods, which should be included for complete coverage.
describe("get redirectURL", () => {
it("should return the redirectURL", () => {
const msg = new MsgSignInitialTx(mockTransaction, mockRedirectURL, mockPermissions, mockPublicKey);
expect(msg.redirectURL).toEqual(mockRedirectURL);
});
});
+
+ describe("get permissions", () => {
+ it("should return the permissions", () => {
+ const msg = new MsgSignInitialTx(mockTransaction, mockRedirectURL, mockPermissions, mockPublicKey);
+ expect(msg.permissions).toEqual(mockPermissions);
+ });
+ });
+
+ describe("get publicKey", () => {
+ it("should return the publicKey", () => {
+ const msg = new MsgSignInitialTx(mockTransaction, mockRedirectURL, mockPermissions, mockPublicKey);
+ expect(msg.publicKey).toEqual(mockPublicKey);
+ });
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| describe("get transaction", () => { | |
| it("should return the transaction", () => { | |
| const msg = new MsgSignInitialTx(mockTransaction, mockRedirectURL, mockPermissions, mockPublicKey); | |
| expect(msg.transaction).toEqual(mockTransaction); | |
| }); | |
| }); | |
| describe("get redirectURL", () => { | |
| it("should return the redirectURL", () => { | |
| const msg = new MsgSignInitialTx(mockTransaction, mockRedirectURL, mockPermissions, mockPublicKey); | |
| expect(msg.redirectURL).toEqual(mockRedirectURL); | |
| }); | |
| }); | |
| }); | |
| describe("get transaction", () => { | |
| it("should return the transaction", () => { | |
| const msg = new MsgSignInitialTx(mockTransaction, mockRedirectURL, mockPermissions, mockPublicKey); | |
| expect(msg.transaction).toEqual(mockTransaction); | |
| }); | |
| }); | |
| describe("get redirectURL", () => { | |
| it("should return the redirectURL", () => { | |
| const msg = new MsgSignInitialTx(mockTransaction, mockRedirectURL, mockPermissions, mockPublicKey); | |
| expect(msg.redirectURL).toEqual(mockRedirectURL); | |
| }); | |
| }); | |
| describe("get permissions", () => { | |
| it("should return the permissions", () => { | |
| const msg = new MsgSignInitialTx(mockTransaction, mockRedirectURL, mockPermissions, mockPublicKey); | |
| expect(msg.permissions).toEqual(mockPermissions); | |
| }); | |
| }); | |
| describe("get publicKey", () => { | |
| it("should return the publicKey", () => { | |
| const msg = new MsgSignInitialTx(mockTransaction, mockRedirectURL, mockPermissions, mockPublicKey); | |
| expect(msg.publicKey).toEqual(mockPublicKey); | |
| }); | |
| }); | |
| }); |
| it("should throw an error if the transaction is not valid", () => { | ||
| expect(() => TransactionCodec.toURLParam("invalid")).toThrow( | ||
| `${CodecType.TRANSACTION_CODEC}: ${CodecErrorCodes.ERROR_ENCODING_URL}`, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Fix incorrect test case for invalid transaction object.
This test is attempting to pass a string to a method that expects a Transaction object, which doesn't correctly test the invalid transaction case.
- it("should throw an error if the transaction is not valid", () => {
- expect(() => TransactionCodec.toURLParam("invalid")).toThrow(
+ it("should throw an error if the transaction is not a valid Transaction object", () => {
+ expect(() => TransactionCodec.toURLParam({} as any)).toThrow(
`${CodecType.TRANSACTION_CODEC}: ${CodecErrorCodes.ERROR_ENCODING_URL}`,
);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it("should throw an error if the transaction is not valid", () => { | |
| expect(() => TransactionCodec.toURLParam("invalid")).toThrow( | |
| `${CodecType.TRANSACTION_CODEC}: ${CodecErrorCodes.ERROR_ENCODING_URL}`, | |
| ); | |
| }); | |
| it("should throw an error if the transaction is not a valid Transaction object", () => { | |
| expect(() => TransactionCodec.toURLParam({} as any)).toThrow( | |
| `${CodecType.TRANSACTION_CODEC}: ${CodecErrorCodes.ERROR_ENCODING_URL}`, | |
| ); | |
| }); |
| const urlObj = new URL(url); | ||
| urlObj.searchParams.set("transaction", TransactionCodec.toURLParam(this._transaction)); | ||
| urlObj.searchParams.set("redirectURL", this._redirectURL); | ||
|
|
||
| return urlObj.toString(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle potential exceptions in toURL method
The toURL method calls TransactionCodec.toURLParam() which might throw exceptions if encoding fails, but these exceptions aren't caught or handled. Additionally, there's no handling for invalid base URLs.
Add error handling to manage potential failures:
toURL(url: string): string {
+ try {
const urlObj = new URL(url);
urlObj.searchParams.set("transaction", TransactionCodec.toURLParam(this._transaction));
urlObj.searchParams.set("redirectURL", this._redirectURL);
return urlObj.toString();
+ } catch (error) {
+ // If the error is from TransactionCodec, let it propagate
+ if (error instanceof CodecError) {
+ throw error;
+ }
+ throw new MsgError(MsgErrorCodes.INVALID_URL);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const urlObj = new URL(url); | |
| urlObj.searchParams.set("transaction", TransactionCodec.toURLParam(this._transaction)); | |
| urlObj.searchParams.set("redirectURL", this._redirectURL); | |
| return urlObj.toString(); | |
| toURL(url: string): string { | |
| try { | |
| const urlObj = new URL(url); | |
| urlObj.searchParams.set("transaction", TransactionCodec.toURLParam(this._transaction)); | |
| urlObj.searchParams.set("redirectURL", this._redirectURL); | |
| return urlObj.toString(); | |
| } catch (error) { | |
| // If the error is from TransactionCodec, let it propagate | |
| if (error instanceof CodecError) { | |
| throw error; | |
| } | |
| throw new MsgError(MsgErrorCodes.INVALID_URL); | |
| } | |
| } |
| static fromURL(url: string): MsgFakSign { | ||
| const urlObj = new URL(url); | ||
| const encodedTransaction = urlObj.searchParams.get("transaction"); | ||
| const redirectURL = urlObj.searchParams.get("redirectURL"); | ||
|
|
||
| if (!encodedTransaction || !redirectURL) { | ||
| throw new MsgError(MsgErrorCodes.INVALID_URL); | ||
| } | ||
|
|
||
| const transaction = TransactionCodec.fromURLParam(encodedTransaction); | ||
| return new MsgFakSign(transaction, redirectURL); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in fromURL method
The current implementation only checks if parameters exist but doesn't handle potential exceptions from:
- The URL constructor if an invalid URL is provided
- The TransactionCodec.fromURLParam method if transaction decoding fails
Improve error handling by catching and properly handling these potential exceptions:
static fromURL(url: string): MsgFakSign {
+ try {
const urlObj = new URL(url);
const encodedTransaction = urlObj.searchParams.get("transaction");
const redirectURL = urlObj.searchParams.get("redirectURL");
if (!encodedTransaction || !redirectURL) {
throw new MsgError(MsgErrorCodes.INVALID_URL);
}
const transaction = TransactionCodec.fromURLParam(encodedTransaction);
return new MsgFakSign(transaction, redirectURL);
+ } catch (error) {
+ if (error instanceof MsgError) {
+ throw error;
+ }
+ // If the error is from TransactionCodec, let it propagate
+ if (error instanceof CodecError) {
+ throw error;
+ }
+ throw new MsgError(MsgErrorCodes.INVALID_URL);
+ }
}Note: This implementation assumes there's a CodecError class that TransactionCodec throws. If that's not the case, you might need to adjust the error handling accordingly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static fromURL(url: string): MsgFakSign { | |
| const urlObj = new URL(url); | |
| const encodedTransaction = urlObj.searchParams.get("transaction"); | |
| const redirectURL = urlObj.searchParams.get("redirectURL"); | |
| if (!encodedTransaction || !redirectURL) { | |
| throw new MsgError(MsgErrorCodes.INVALID_URL); | |
| } | |
| const transaction = TransactionCodec.fromURLParam(encodedTransaction); | |
| return new MsgFakSign(transaction, redirectURL); | |
| } | |
| static fromURL(url: string): MsgFakSign { | |
| try { | |
| const urlObj = new URL(url); | |
| const encodedTransaction = urlObj.searchParams.get("transaction"); | |
| const redirectURL = urlObj.searchParams.get("redirectURL"); | |
| if (!encodedTransaction || !redirectURL) { | |
| throw new MsgError(MsgErrorCodes.INVALID_URL); | |
| } | |
| const transaction = TransactionCodec.fromURLParam(encodedTransaction); | |
| return new MsgFakSign(transaction, redirectURL); | |
| } catch (error) { | |
| if (error instanceof MsgError) { | |
| throw error; | |
| } | |
| // If the error is from TransactionCodec, let it propagate | |
| if (error instanceof CodecError) { | |
| throw error; | |
| } | |
| throw new MsgError(MsgErrorCodes.INVALID_URL); | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (18)
packages/shared/core/src/modules/common/msgs/msg.error.ts (1)
1-2: Consider using explicit import instead of index importWhile importing from "." works, it creates an implicit dependency on the barrel file. For better maintainability and to avoid potential circular dependencies, consider importing directly from "./error-codes".
-import { MsgErrorCodes } from "."; +import { MsgErrorCodes } from "./error-codes";packages/shared/core/src/modules/base/clients/i-near-dapp.client.ts (2)
1-2: Consider separating imports for better readabilityWhen importing multiple items, it's often clearer to import them on separate lines, especially as the list grows.
-import { MsgFakSign, MsgSignIn } from "../msgs"; +import { MsgFakSign } from "../msgs"; +import { MsgSignIn } from "../msgs";
13-18: Method name could be more descriptiveThe method name
signWithFakis not immediately clear to those unfamiliar with the codebase. Consider a more descriptive name likesignWithFakeSignatureor add a more detailed JSDoc explaining what "Fak" stands for.packages/shared/core/src/modules/base/msgs/fak-sign.msg.ts (2)
42-44: Parameter name 'url' in toURL method is misleading.The method parameter 'url' suggests it's the complete URL, but it's actually used as a base URL to which you're adding query parameters. Consider renaming it to 'baseURL' for clarity.
- toURL(url: string): string { - const urlObj = new URL(url); + toURL(baseURL: string): string { + const urlObj = new URL(baseURL);
27-38: Add URL validation in fromURL method.Currently, the method assumes the URL string is valid. Consider adding try/catch to handle potential URL parsing errors that could occur with malformed inputs.
static fromURL(url: string): MsgFakSign { + try { const urlObj = new URL(url); const encodedTransaction = urlObj.searchParams.get("transaction"); const redirectURL = urlObj.searchParams.get("redirectURL"); if (!encodedTransaction || !redirectURL) { throw new MsgError(MsgErrorCodes.INVALID_URL); } const transaction = TransactionCodec.fromURLParam(encodedTransaction); return new MsgFakSign(transaction, redirectURL); + } catch (error) { + if (error instanceof MsgError) throw error; + throw new MsgError(MsgErrorCodes.INVALID_URL); + } }packages/shared/core/src/modules/base/msgs/sign-initial-tx.msg.ts (4)
37-37: Fix typo in variable name.There's a typo in the variable name 'encodedTrasaction' which should be 'encodedTransaction'.
- const encodedTrasaction = urlObj.searchParams.get("transaction"); + const encodedTransaction = urlObj.searchParams.get("transaction");
12-12: Use a more specific type for permissions property.The
_permissionsproperty is typed asobject, which is very general. Consider defining a more specific interface that captures the structure of the permissions.- _permissions: object; + _permissions: { + [key: string]: boolean | string | number | object; + };Alternatively, create a dedicated interface for permissions:
interface Permissions { // Add specific permission properties here [key: string]: boolean | string | number | object; }Then update the class to use this interface.
58-66: Parameter name 'url' in toURL method is misleading.Similar to the other message classes, the parameter 'url' suggests it's the complete URL, but it's actually used as a base URL to which you're adding query parameters. Consider renaming it to 'baseURL' for clarity.
- toURL(url: string): string { - const urlObj = new URL(url); + toURL(baseURL: string): string { + const urlObj = new URL(baseURL);
34-51: Add URL validation in fromURL method.Similar to the recommendation for MsgFakSign, consider adding try/catch to handle potential URL parsing errors for malformed inputs.
static fromURL(url: string): MsgSignInitialTx { + try { const urlObj = new URL(url); const encodedTransaction = urlObj.searchParams.get("transaction"); const redirectURL = urlObj.searchParams.get("redirectURL"); const urlPermissions = urlObj.searchParams.get("permissions"); const urlPK = urlObj.searchParams.get("publicKey"); if (!encodedTransaction || !redirectURL || !urlPermissions || !urlPK) { throw new MsgError(MsgErrorCodes.INVALID_URL); } const transaction = TransactionCodec.fromURLParam(encodedTransaction); const permissions = PermissionsCodec.fromURLParam(urlPermissions); const publicKey = PublicKeyCodec.fromURLParam(urlPK); return new MsgSignInitialTx(transaction, redirectURL, permissions, publicKey); + } catch (error) { + if (error instanceof MsgError) throw error; + throw new MsgError(MsgErrorCodes.INVALID_URL); + } }packages/shared/core/src/modules/relayer/msgs/relay-sign-in.msg.ts (3)
46-52: Use consistent property access approach.While other message classes directly access private properties (this._property), this method uses getters (this.property). Choose one approach for consistency across all message classes.
toURL(url: string): string { const urlObj = new URL(url); - urlObj.searchParams.set("accountID", this.accountID); - urlObj.searchParams.set("signingURL", this.signingURL); - urlObj.searchParams.set("relayerAPI", this.relayerAPI); + urlObj.searchParams.set("accountID", this._accountID); + urlObj.searchParams.set("signingURL", this._signingURL); + urlObj.searchParams.set("relayerAPI", this._relayerAPI); return urlObj.toString(); }
46-52: Parameter name 'url' in toURL method is misleading.Similar to the other message classes, rename 'url' to 'baseURL' for clarity.
- toURL(url: string): string { - const urlObj = new URL(url); + toURL(baseURL: string): string { + const urlObj = new URL(baseURL);
28-39: Add URL validation in fromURL method.Similar to other message classes, add try/catch to handle potential URL parsing errors for malformed inputs.
static fromURL(url: string): MsgRelaySignIn { + try { const urlObj = new URL(url); const accountID = urlObj.searchParams.get("accountID"); const signingURL = urlObj.searchParams.get("signingURL"); const relayerAPI = urlObj.searchParams.get("relayerAPI"); if (!accountID || !signingURL || !relayerAPI) { throw new MsgError(MsgErrorCodes.INVALID_URL); } return new MsgRelaySignIn(accountID, signingURL, relayerAPI); + } catch (error) { + if (error instanceof MsgError) throw error; + throw new MsgError(MsgErrorCodes.INVALID_URL); + } }packages/shared/core/src/modules/base/msgs/sign-in.msg.ts (3)
6-9: Update example in JSDoc to match actual API.The example shows calling
toURL()without parameters, but the actual method requires a URL parameter.* @example * ``` * const msg = new MsgSignIn("accountID", "signingURL"); -* const url = msg.toURL(); +* const url = msg.toURL("https://example.com"); * ```
47-52: Parameter name 'url' in toURL method is misleading.Similar to the other message classes, rename 'url' to 'baseURL' for clarity.
- toURL(url: string): string { - const urlObj = new URL(url); + toURL(baseURL: string): string { + const urlObj = new URL(baseURL);
30-40: Add URL validation in fromURL method.Similar to other message classes, add try/catch to handle potential URL parsing errors for malformed inputs.
static fromURL(url: string): MsgSignIn { + try { const urlObj = new URL(url); const accountID = urlObj.searchParams.get("accountID"); const signingURL = urlObj.searchParams.get("signingURL"); if (!accountID || !signingURL) { throw new MsgError(MsgErrorCodes.INVALID_URL); } return new MsgSignIn(accountID, signingURL); + } catch (error) { + if (error instanceof MsgError) throw error; + throw new MsgError(MsgErrorCodes.INVALID_URL); + } }packages/shared/core/package.json (3)
16-17: New Dev Dependency Added:The addition of
"@shared/test": "workspace:*"in thedevDependenciessection hints at new testing utilities or shared configurations. Ensure that this dependency is available in the workspace and that its presence aligns with your overall testing strategy.
20-22: Dependency Version Consistency Check:The inclusion of
"jest": "^29.7.0","near-api-js": "^5.1.1", and"ts-jest": "^29.2.5"intodevDependenciesis now clearly defined. Note that"near-api-js"is also specified inpeerDependencies. Verify that both the development and consuming environments will use compatible versions to avoid potential conflicts.
24-26: Peer Dependency Specification:The new
peerDependenciessection requiring"near-api-js": "^5.1.1"helps enforce version consistency for consumers of this package. Confirm that this version requirement is aligned with the broader project’s ecosystem and that consumers are prepared to install this dependency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
packages/shared/core/package.json(2 hunks)packages/shared/core/src/modules/base/clients/i-near-dapp.client.ts(1 hunks)packages/shared/core/src/modules/base/clients/i-near-wallet.client.ts(1 hunks)packages/shared/core/src/modules/base/clients/index.ts(1 hunks)packages/shared/core/src/modules/base/index.ts(1 hunks)packages/shared/core/src/modules/base/msgs/fak-sign.msg.ts(1 hunks)packages/shared/core/src/modules/base/msgs/index.ts(1 hunks)packages/shared/core/src/modules/base/msgs/sign-in.msg.ts(1 hunks)packages/shared/core/src/modules/base/msgs/sign-initial-tx.msg.ts(1 hunks)packages/shared/core/src/modules/common/index.ts(1 hunks)packages/shared/core/src/modules/common/msgs/error-codes.ts(1 hunks)packages/shared/core/src/modules/common/msgs/index.ts(1 hunks)packages/shared/core/src/modules/common/msgs/msg.error.ts(1 hunks)packages/shared/core/src/modules/index.ts(1 hunks)packages/shared/core/src/modules/relayer/clients/i-near-relayer-wallet.client.ts(1 hunks)packages/shared/core/src/modules/relayer/index.ts(1 hunks)packages/shared/core/src/modules/relayer/msgs/relay-sign-in.msg.ts(1 hunks)
✅ Files skipped from review due to trivial changes (6)
- packages/shared/core/src/modules/common/index.ts
- packages/shared/core/src/modules/relayer/index.ts
- packages/shared/core/src/modules/base/clients/index.ts
- packages/shared/core/src/modules/base/msgs/index.ts
- packages/shared/core/src/modules/base/index.ts
- packages/shared/core/src/modules/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/shared/core/src/modules/common/msgs/index.ts
- packages/shared/core/src/modules/relayer/clients/i-near-relayer-wallet.client.ts
🧰 Additional context used
🧬 Code Graph Analysis (6)
packages/shared/core/src/modules/base/msgs/sign-initial-tx.msg.ts (5)
packages/shared/core/src/modules/base/msgs/fak-sign.msg.ts (2)
transaction(57-59)redirectURL(65-67)packages/shared/core/src/modules/common/msgs/msg.error.ts (1)
MsgError(6-14)packages/shared/core/src/modules/common/codecs/transaction.codec.ts (1)
TransactionCodec(8-36)packages/shared/core/src/modules/common/codecs/permissions.codec.ts (1)
PermissionsCodec(7-33)packages/shared/core/src/modules/common/codecs/public-key.codec.ts (1)
PublicKeyCodec(8-34)
packages/shared/core/src/modules/base/clients/i-near-dapp.client.ts (2)
packages/shared/core/src/modules/base/msgs/sign-in.msg.ts (1)
MsgSignIn(11-69)packages/shared/core/src/modules/base/msgs/fak-sign.msg.ts (1)
MsgFakSign(8-68)
packages/shared/core/src/modules/base/clients/i-near-wallet.client.ts (1)
packages/shared/core/src/modules/base/msgs/sign-in.msg.ts (1)
MsgSignIn(11-69)
packages/shared/core/src/modules/relayer/msgs/relay-sign-in.msg.ts (2)
packages/shared/core/src/modules/base/msgs/sign-in.msg.ts (2)
accountID(66-68)signingURL(58-60)packages/shared/core/src/modules/common/msgs/msg.error.ts (1)
MsgError(6-14)
packages/shared/core/src/modules/base/msgs/fak-sign.msg.ts (3)
packages/shared/core/src/modules/base/msgs/sign-initial-tx.msg.ts (2)
transaction(72-74)redirectURL(80-82)packages/shared/core/src/modules/common/msgs/msg.error.ts (1)
MsgError(6-14)packages/shared/core/src/modules/common/codecs/transaction.codec.ts (1)
TransactionCodec(8-36)
packages/shared/core/src/modules/base/msgs/sign-in.msg.ts (2)
packages/shared/core/src/modules/relayer/msgs/relay-sign-in.msg.ts (2)
accountID(58-60)signingURL(66-68)packages/shared/core/src/modules/common/msgs/msg.error.ts (1)
MsgError(6-14)
🔇 Additional comments (6)
packages/shared/core/src/modules/common/msgs/error-codes.ts (1)
1-3: Well-structured error codes implementationThis enum provides a clean way to define message error codes. Using string values makes the errors more readable in logs and debugging sessions.
packages/shared/core/src/modules/common/msgs/msg.error.ts (1)
6-14: Good error class implementationThe error class is well-structured with proper extension of the built-in Error class and a readonly code property. The JSDoc comments are clear and helpful.
packages/shared/core/src/modules/base/clients/i-near-wallet.client.ts (1)
1-12: Clean interface implementation for wallet clientThe interface is well-defined with proper JSDoc documentation. The method signature clearly shows that it's for signing in with a MsgSignIn object.
packages/shared/core/src/modules/base/msgs/fak-sign.msg.ts (1)
1-68: Well-structured implementation of the fake signing message class.The class is well-documented with JSDoc comments and follows a consistent naming convention.
packages/shared/core/package.json (2)
3-3: Version Number Update Observation:The package version is now set to
"0.0.0". Please confirm that this reset is intentional (e.g., for pre-release or internal versioning reasons) so that downstream consumers are aware of the versioning scheme.
27-32:Details
❓ Verification inconclusive
Exports Field Definition:
The newly added
exportsfield maps module entry points (e.g.,"./codecs","./base", etc.) to TypeScript source files. Typically, packages publish compiled JavaScript files for distribution. If this package is consumed directly as TypeScript or has a custom build setup that compiles on consumption, please ensure that the configuration handles it appropriately. Otherwise, consider pointing these exports to the compiled output to avoid runtime issues.
Below is a shell script to extract key configuration details from the
packages/shared/core/package.jsonfile. This will help verify whether the package’s exports mapping to TypeScript source files is intentional (for example, if the package is consumed directly as TypeScript or if a build step that compiles to JavaScript is already defined):Please run this script and share the results so we can verify if the current export paths are appropriate or if they need to point to compiled JavaScript output.
🏁 Script executed:
#!/bin/bash echo "Inspecting packages/shared/core/package.json for main, types, exports, and build script configuration..." jq '{ main: .main, types: .types, exports: .exports, scripts: .scripts }' packages/shared/core/package.jsonLength of output: 801
Review Verification: Confirm the Intended Consumption of TS Sources
The
exportsfield inpackages/shared/core/package.jsoncurrently maps module entry points to TypeScript source files (e.g.,"./codecs": "./src/modules/common/codecs/index.ts", etc.). Given that both"main"and"types"arenulland that the build script ("build": "npx tsc") is available—but not reflected in these export paths—it’s important to verify the intended consumption pattern:
- If consumers are expected to import the raw TypeScript sources (for instance, within a monorepo where TypeScript compilation happens at consumption), then this configuration might be acceptable.
- Otherwise, if the package is meant for distribution as compiled JavaScript, please update the exports to point to the compiled output (e.g., files under a
/distdirectory) and ensure that"main"and"types"are set accordingly to prevent potential runtime issues.Please validate that the current setup matches your intended publishing strategy.
| throw new MsgError(MsgErrorCodes.INVALID_URL); | ||
| } | ||
|
|
||
| const transaction = TransactionCodec.fromURLParam(encodedTrasaction); |
There was a problem hiding this comment.
Update variable reference after fixing typo.
This line also needs to be updated to use the corrected variable name.
- const transaction = TransactionCodec.fromURLParam(encodedTrasaction);
+ const transaction = TransactionCodec.fromURLParam(encodedTransaction);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const transaction = TransactionCodec.fromURLParam(encodedTrasaction); | |
| const transaction = TransactionCodec.fromURLParam(encodedTransaction); |
| const urlPermissions = urlObj.searchParams.get("permissions"); | ||
| const urlPK = urlObj.searchParams.get("publicKey"); | ||
|
|
||
| if (!encodedTrasaction || !redirectURL || !urlPermissions || !urlPK) { |
There was a problem hiding this comment.
Update variable reference after fixing typo.
This line needs to be updated to use the corrected variable name.
- if (!encodedTrasaction || !redirectURL || !urlPermissions || !urlPK) {
+ if (!encodedTransaction || !redirectURL || !urlPermissions || !urlPK) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!encodedTrasaction || !redirectURL || !urlPermissions || !urlPK) { | |
| if (!encodedTransaction || !redirectURL || !urlPermissions || !urlPK) { |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/shared/core/src/modules/base/clients/i-near-dapp.client.ts (1)
19-23: Method naming consistency with message type is now appropriateThe method
signInitialTxnow correctly accepts a parameter of typeMsgSignInitialTx, which resolves the previous inconsistency between the method name and parameter type.
🧹 Nitpick comments (2)
packages/shared/core/src/modules/base/clients/i-near-dapp.client.ts (2)
13-17: Fix incorrect JSDoc commentThe JSDoc comment for the
signingURL()method incorrectly states it "Returns the account ID" when it should state it returns the signing URL./** - * Returns the account ID. + * Returns the signing URL. * @returns The signing URL. */
25-29: Consider renamingsignWithFakmethod for clarityThe method name
signWithFakis not immediately clear to developers unfamiliar with the codebase. While the JSDoc explains it's for fake signatures, a more descriptive name likesignWithFakeSignatureorsignWithFakSignaturewould improve readability and maintenance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/shared/core/src/modules/base/clients/i-near-dapp.client.ts(1 hunks)packages/shared/core/src/modules/relayer/clients/i-near-relayer-dapp.client.ts(1 hunks)packages/shared/core/src/modules/relayer/clients/index.ts(1 hunks)packages/shared/core/test/modules/common/msgs/fak-sign.msg.spec.ts(1 hunks)packages/shared/core/test/modules/common/msgs/sign-in.msg.spec.ts(1 hunks)packages/shared/core/test/modules/common/msgs/sign-initial-tx.msg.spec.ts(1 hunks)packages/shared/core/test/modules/relayer/msgs/relay-sign-in.msg.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/shared/core/test/modules/relayer/msgs/relay-sign-in.msg.spec.ts
- packages/shared/core/test/modules/common/msgs/fak-sign.msg.spec.ts
- packages/shared/core/test/modules/common/msgs/sign-in.msg.spec.ts
- packages/shared/core/src/modules/relayer/clients/index.ts
- packages/shared/core/test/modules/common/msgs/sign-initial-tx.msg.spec.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/shared/core/src/modules/relayer/clients/i-near-relayer-dapp.client.ts (1)
packages/shared/core/src/modules/base/clients/i-near-dapp.client.ts (1)
INearDAppClient(6-30)
packages/shared/core/src/modules/base/clients/i-near-dapp.client.ts (2)
packages/shared/core/src/modules/base/msgs/sign-initial-tx.msg.ts (1)
MsgSignInitialTx(9-99)packages/shared/core/src/modules/base/msgs/fak-sign.msg.ts (1)
MsgFakSign(8-68)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-base / Build
🔇 Additional comments (1)
packages/shared/core/src/modules/relayer/clients/i-near-relayer-dapp.client.ts (1)
3-9: Interface extension looks goodThe
INearRelayerDAppClientinterface appropriately extends the baseINearDAppClientinterface and adds a clearly definedrelayerAPI()method with proper documentation. This follows good object-oriented design principles by specializing the base client for relayer-specific functionality.
[TA-4489]: sign with full access key (dapp)
[TA-4291]: sign in flow
2 participants
[TA-4419]: core messages definition
Changes 🛠️
shared/core
MsgSignIn,MsgSignInitialTx,MsgFakSignandMsgRelayerSignInmessagesTransactionCodec,PermissionsCodecandPublicKeyCodeccodecs.core/commonclients:INearDAppClientandINearWalletClient.core/relayerclients:INearRelayerWalletClient.Summary by CodeRabbit
New Features
Chores