Skip to content

feat: add signMessage utility with multi-chain support #34

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat: add signMessage utility with multi-chain support #34

wants to merge 3 commits into from

Conversation

0binna-oss
Copy link

@0binna-oss 0binna-oss commented Mar 28, 2025 •
edited
Loading

Description

Adds a signMessage utility to sign messages with a private key (EVM supported) .

Please include a summary of the changes and be sure to follow our Contribution Guidelines.

Type of change/Updated Implementation:

  • Moved logic to ethereumHelper.ts and solanaHelper.ts as requested
  • Removed standalone signMessage.ts
  • Maintained all original functionality
  • New feature (non-breaking change which adds functionality)

Checklist:

  • I have selected the correct base branch.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • Any dependent changes have been merged and published in downstream modules.
  • I ran npm run test and my test cases cover all the lines and branches of the added code.
  • I ran npm run build with success.
  • I have tested my code on the live network.
  • I have checked the Deploy Preview and it looks correct.

@iamnotstatic
Copy link
Owner

Description

Adds a signMessage utility to sign messages with a private key (EVM supported) .

Please include a summary of the changes and be sure to follow our Contribution Guidelines.

Type of change

  • New feature (non-breaking change which adds functionality)

Checklist:

  • I have selected the correct base branch.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • Any dependent changes have been merged and published in downstream modules.
  • I ran npm run test and my test cases cover all the lines and branches of the added code.
  • I ran npm run build with success.
  • I have tested my code on the live network.
  • I have checked the Deploy Preview and it looks correct.

Hi @0binna-oss , this looks good! But can you follow the same pattern as the existing method? Please put the sign for Solana in src/common/helpers/solanaHelper.ts, same with the Ethereum helper, which covers all EVMS, etc.

@0binna-oss
Copy link
Author

Good day @iamnotstatic , will look into it as soon as possible.

@0binna-oss
Copy link
Author

Good day @iamnotstatic , I've restructured the code as requested. Please review

@iamnotstatic iamnotstatic added the enhancement New feature or request label Apr 5, 2025
@iamnotstatic iamnotstatic self-requested a review April 18, 2025 08:12
iamnotstatic
iamnotstatic requested changes Apr 18, 2025
View reviewed changes
README.md
@@ -936,6 +936,27 @@ The optional parameters that the object takes in are: value, contractAbi, gas pr
}
```

### Signing Message
Copy link
Owner

Choose a reason for hiding this comment

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

Hi @0binna-oss, this readme is not following the existing structure Can you update it ?

README.md Outdated
const solanaSignature = await signMessage("hello", solanaKey, "solana");


### Updated Implementation
Copy link
Owner

Choose a reason for hiding this comment

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

i beleive this was suppose to be a commit message not for the documentation

src/utils/signMessage.js Outdated
import { ethers } from "ethers";
import { Keypair } from "@solana/web3.js";

export async function signMessage(message, privateKey, chainType = "evm") {
Copy link
Owner

Choose a reason for hiding this comment

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

We don't need this, it should reference the src/services/wallet/index.ts just like every other function

test/ethereumHelper.test.ts Outdated
@@ -0,0 +1,21 @@
import { signEvmMessage } from "../src/common/helpers/ethereumHelper";
Copy link
Owner

Choose a reason for hiding this comment

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

Let the test be directly in the dedicated test file for everything chain
wallet.ethereum.test.ts
wallet.solana.test.ts

@iamnotstatic
Copy link
Owner

Good day @iamnotstatic , I've restructured the code as requested. Please review

Thanks for the update @0binna-oss, dropped some comments

@0binna-oss
Copy link
Author

Good day @iamnotstatic, made updates as requested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iamnotstatic iamnotstatic iamnotstatic requested changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@0binna-oss @iamnotstatic