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 batch transaction UX #7473

Merged
merged 4 commits into from
Jan 10, 2020
Merged

Fix batch transaction UX #7473

merged 4 commits into from
Jan 10, 2020

Conversation

rekmarks
Copy link
Member

@rekmarks rekmarks commented Nov 19, 2019

Pending

Fixes batch transaction UX by addressing long-standing errors in batch transaction handling in the UI, and modifying json-rpc-engine's ordered batch request handling. Transactions are now ordered from oldest to newest in the background and in the UI. Users can still freely navigate back and forth.

Fixes #5852

@danjm danjm mentioned this pull request Nov 20, 2019
app/scripts/controllers/transactions/index.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
Default to showing the earliest created transaction when routing to confirm screens
@rekmarks rekmarks requested review from whymarrh and Gudahtt January 8, 2020 22:45
@rekmarks rekmarks added area-UI Relating to the user interface. area-background Issues relating to the extension background process. area-transactions labels Jan 8, 2020
@rekmarks
Copy link
Member Author

rekmarks commented Jan 8, 2020

Just re-requested reviews, some tests are failing but unclear if due to my changes. Help appreciated.

Ready for review. Thanks to @danjm and @Gudahtt

@rekmarks rekmarks force-pushed the update-rpc-engine branch 2 times, most recently from d9b8d53 to b599858 Compare January 9, 2020 16:53
Comment on lines -856 to +858
await driver.wait(until.elementTextMatches(balance, /^87.*\s*ETH.*$/), 10000)
await driver.wait(until.elementTextMatches(balance, /^90.*\s*ETH.*$/), 10000)
const tokenAmount = await balance.getText()
assert.ok(/^87.*\s*ETH.*$/.test(tokenAmount))
assert.ok(/^90.*\s*ETH.*$/.test(tokenAmount))
Copy link
Member Author

Choose a reason for hiding this comment

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

These changes are due to a send of 3 ETH now being rejected instead of approved due to the updated UI transaction order. An equivalent solution would be to ensure that transaction is approved, this was just easier.

@metamaskbot
Copy link
Collaborator

Builds ready [5ed8494]

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Copy link
Contributor

@whymarrh whymarrh left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -26,7 +26,7 @@ const typeHash = {

const Button = ({ type, submit, large, children, className, ...buttonProps }) => (
<button
type={submit && 'submit'}
type={submit ? 'submit' : undefined}
Copy link
Contributor

Choose a reason for hiding this comment

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

🙏

@rekmarks rekmarks merged commit 728026d into develop Jan 10, 2020
@rekmarks rekmarks deleted the update-rpc-engine branch January 10, 2020 14:34
Gudahtt pushed a commit that referenced this pull request Jan 10, 2020
* order transactions from oldest to newest in UI

* update json-rpc-engine, eth-json-rpc-middleware

* update e2e and integration tests
Gudahtt added a commit that referenced this pull request Jan 10, 2020
These tests were updated in #7473 to navigate in a different order,
because the transaction order changed. Unfortunately this meant that
a second contract deployment was being confirmed, where it was
previously being rejected.

This updates the test to ensure the same transaction is rejected and
confirmed as prior to the change in #7473
Gudahtt added a commit that referenced this pull request Jan 10, 2020
These tests were updated in #7473 to navigate in a different order,
because the transaction order changed. Unfortunately this meant that
a second contract deployment was being confirmed, where it was
previously being rejected.

This updates the test to ensure the same transaction is rejected and
confirmed as prior to the change in #7473
Gudahtt added a commit that referenced this pull request Jan 10, 2020
These tests were updated in #7473 to navigate in a different order,
because the transaction order changed. Unfortunately this meant that
a second contract deployment was being confirmed, where it was
previously being rejected.

This updates the test to ensure the same transaction is rejected and
confirmed as prior to the change in #7473
Gudahtt added a commit that referenced this pull request Jan 13, 2020
These tests were updated in #7473 to navigate in a different order,
because the transaction order changed. Unfortunately this meant that
a second contract deployment was being confirmed, where it was
previously being rejected.

This updates the test to ensure the same transaction is rejected and
confirmed as prior to the change in #7473
yqrashawn pushed a commit to yqrashawn/conflux-portal that referenced this pull request Jan 14, 2020
* order transactions from oldest to newest in UI

* update json-rpc-engine, eth-json-rpc-middleware

* update e2e and integration tests
yqrashawn pushed a commit to yqrashawn/conflux-portal that referenced this pull request Jan 15, 2020
These tests were updated in MetaMask#7473 to navigate in a different order,
because the transaction order changed. Unfortunately this meant that
a second contract deployment was being confirmed, where it was
previously being rejected.

This updates the test to ensure the same transaction is rejected and
confirmed as prior to the change in MetaMask#7473
yqrashawn pushed a commit to Conflux-Chain/conflux-portal that referenced this pull request Jan 15, 2020
* order transactions from oldest to newest in UI

* update json-rpc-engine, eth-json-rpc-middleware

* update e2e and integration tests
yqrashawn pushed a commit to Conflux-Chain/conflux-portal that referenced this pull request Jan 15, 2020
These tests were updated in MetaMask#7473 to navigate in a different order,
because the transaction order changed. Unfortunately this meant that
a second contract deployment was being confirmed, where it was
previously being rejected.

This updates the test to ensure the same transaction is rejected and
confirmed as prior to the change in MetaMask#7473
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-background Issues relating to the extension background process. area-transactions area-UI Relating to the user interface.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

batch transactions appear in wrong order and fail my transactions
5 participants