Skip to content
This repository has been archived by the owner on Mar 3, 2021. It is now read-only.

Makes plugin sendTransaction work with non-vm providers, adds tests #1301

Merged
merged 1 commit into from
Sep 27, 2019

Conversation

bmcd
Copy link
Contributor

@bmcd bmcd commented Sep 23, 2019

Even though #1265 fixed silentRunTx to allow non-VM transactions, I found some issues while trying to use udapp.sendTransaction from a plugin:

  • The tx would go through when using Web3 Provider, but in the final callback there were some VM-specific result values that were being used, causing it to throw an error.
  • This error wasn't being sent to the reject() function in the promise, so the plugin just got undefined and didn't know what the error was.
  • The result object that the callback for silentRunTx() has 3 different forms depending on the environment and transaction type, but only the VM type was handled.

This PR adds:

  • a try/catch to send any errors to reject().
  • a helper method that converts the different possible result types into the expected RemixTx object.
  • UniveralDapp didn't have any test coverage, so I just made tests for the helper method it calls, with example result objects that I got during testing.

Please let me know if you want me to change anything. Thanks!

Copy link
Collaborator

@Aniket-Engg Aniket-Engg 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 this @bmcd87 . Almost everything looks fine.

remix-lib/src/universalDapp.js Show resolved Hide resolved
remix-lib/src/helpers/txResultHelper.js Show resolved Hide resolved
@Aniket-Engg Aniket-Engg merged commit 7ea3f1d into ethereum:master Sep 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants