Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: estimateMessageFee should handle from_address as checksum'ed eth address #1054

Conversation

SergeyVolynkin
Copy link

@SergeyVolynkin SergeyVolynkin commented Apr 2, 2024

Motivation and Resolution

While using estimateMessageFee in our deposit/withdraw code, we encountered that passing from_address: 0x01FB18b564c106873ea2951F0f099Af7527E8680 argument to estimateMessageFee causes the following error:

Caused by: LibraryError: RPC: starknet_estimateMessageFee with params {"message":{"from_address":"0x1fb18b564c106873ea2951f0f099af7527e8680","to_address":"0x1e58499da87cf11a2bc75b035f13c5ca3dd7be2ccf6e381714c879b3caa64f6","entry_point_selector":"0x2d757788a8d8d6f21d1cd40bce38a8222d70654214e96ff95d8086e684fbee5","payload":["0x3ab1ead7a1fe8b63ca757ae094268b1f1287a95b36f262bc9ed4ee1900849f7","0x2faf080","0x0"]},"block_id":"pending"}
 -32602: Invalid params: {"reason":"invalid length 39, expected a (both 0x-prefixed or not) hex string or byte array containing 20 bytes at line 1 column 70"}

From the source code, starknet.js seem to convert the hex string to bigint and then back to hex which loses the leading 0x0… our from_address address, which causes incorrect from_address address to be sent

RPC version (if applicable)

v0_6, v0_7

Usage related changes

No breaking changes

Development related changes

No breaking changes

Checklist:

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

The `getChecksumAddress` util function is not for Eth addresses. Add `web3.utils.toChecksumAddress` as proposal, final approach TBD
Copy link
Collaborator

@penovicp penovicp left a comment

Choose a reason for hiding this comment

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

This was reported in #1028 and addressed with the #1040 PR. The PR was merged to the next-version branch which should be merged to develop soon.

In any case, thank you for being proactive about the issue.

@SergeyVolynkin
Copy link
Author

Closing in favor of #1040, thank you @penovicp

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

Successfully merging this pull request may close these issues.

2 participants