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 sending evm tx with Acala #1090

Merged
merged 2 commits into from
Oct 26, 2023
Merged

fix sending evm tx with Acala #1090

merged 2 commits into from
Oct 26, 2023

Conversation

shunjizhan
Copy link
Contributor

@shunjizhan shunjizhan commented Sep 26, 2023

Change

Currently when sending acala evm tx, talisman will throw tx hash mismatch error.

This commit switches to AcalaJsonRpcProvider for Acala network family, which solves the above issue

Test

  • built the wallet locally, and tried sending an Acala evm tx, which succeed and didn't throw tx hash mismatch anymore
  • all CI tests passed

@alecdwm alecdwm self-requested a review September 26, 2023 12:00
@shunjizhan
Copy link
Contributor Author

ummmm it seems there are some compatibility issue, let me take a look into this, will update the PR soon

@shunjizhan
Copy link
Contributor Author

shunjizhan commented Sep 28, 2023

@alecdwm previous issue was that jest didn't seem to respect exports field in package.json, this exports in particular. So I had to manually add the path resolve rule for it, so jest can find this module

Now running yarn test locally passed for me, can you trigger the CI to try again? Thanks!

@alecdwm
Copy link
Member

alecdwm commented Sep 28, 2023

Thanks for the PR! And for the jest fix 🎉

We're just about to release v1.19.0 after some extended testing, so it's unlikely this will make it into that release. But I've heard the plan is to include this PR in a hotfix once we're done with that

Edit:

You can ignore that Talisman CI / Publish a snapshot version... CI check, it shouldn't be running here anyway

@shunjizhan
Copy link
Contributor Author

But I've heard the plan is to include this PR in a hotfix once we're done with that

sounds awesome! Thanks a lot

@chidg chidg self-requested a review October 10, 2023 07:41
Copy link
Contributor

@chidg chidg left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@chidg chidg left a comment

Choose a reason for hiding this comment

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

Thanks for the PR - thought it was good to go however I will have to change my review after further testing
When testing, I found that this PR doesn't work with Talisman's send funds UI when sending Acala substrate assets. The user can't progress to the 'Review' stage, probably because some awaited request has not resolved. The current dev branch is able to send Acala assets in the same way. I wasn't able to dig into the reason. Could you please look into this?

@shunjizhan
Copy link
Contributor Author

shunjizhan commented Oct 10, 2023

@chidg thanks for your review, I tried sending ACA on the substrate side with locally built talisman on this branch, and it worked for me. I was not blocked on the review page, and the tx actually went through.

Were you sending any other assets? Could this happened to be your network issue?

Screenshot 2023-10-10 at 16 43 27 Screenshot 2023-10-10 at 16 43 35 Screenshot 2023-10-10 at 16 43 49

@shunjizhan
Copy link
Contributor Author

shunjizhan commented Oct 10, 2023

also tried sending dot on Acala, and didn't see any issue
Screenshot 2023-10-10 at 16 50 54
Screenshot 2023-10-10 at 16 51 02
Screenshot 2023-10-10 at 16 51 53

@shunjizhan shunjizhan requested a review from chidg October 14, 2023 13:11
@shunjizhan
Copy link
Contributor Author

hey @chidg, any updates on this? Please let me know if there is anything else I can help

@chidg
Copy link
Contributor

chidg commented Oct 23, 2023

Thanks @shunjizhan , I have been AFK for the past couple of weeks. Will have another look this week.

Copy link
Contributor

@chidg chidg left a comment

Choose a reason for hiding this comment

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

Works well, thanks for the neat PR @shunjizhan and apologies for the delay in approving.
LGTM!

@chidg chidg merged commit 704907c into TalismanSociety:dev Oct 26, 2023
3 of 4 checks passed
@shunjizhan
Copy link
Contributor Author

Thanks @chidg! Please keep me updated once this is included in the release

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.

3 participants