-
-
Notifications
You must be signed in to change notification settings - Fork 648
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
Productionize Transaction Stack #456
Conversation
…erWallet into transaction-refactor
…itional inputs to simplify disabled components
add web3 fixes & comments
…tests Transaction Refactor - Saga Tests
…erWallet into transaction-refactor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not experience any bugs sending and unlocking from various wallets, so I'm approving this. The issues I list below could (and probably should) be separate PRs. Looking forward to seeing this land!
Usability Issues
- Unit doesn't explain why it's invalid. Some cases are obvious, but not having enough ETH (especially if it's gas that exceeds it) is one that could use help.
+ Advanced: Add Data
doesn't change text / disappear when expanded. Ignore this though, Improve Gas Price UX #571 should address this more holistically- Sending a transaction should (probably) reset state. At the very least, clear signed tx to prevent double sending the same nonce
General code comments
- Really like the
*Factory
nomenclature and style of components. I think this will make alternative designs and "mini" versions of send really easy to implement. - I don't like the overhead of how many folders are just:
I'd rather make a folder for something if and only if it has sub-components or something, preferring just
Component/ index.tsx Component.tsx
Component.tsx
. Even if it has subcomponents, I think index, MainComponent.tsx, and SubComponent.tsx is sufficient instead of yet another folder of nesting.
Style issues I found are addressed in #615
This reverts commit a40a4c0.
Looks good! Haven't looked through all the code yet to give a thorough review but here's what I've found so far.
|
…erWallet into transaction-refactor
@james-prado Transaction data doesn't get cleared on production site after decline / confirmation. Not sure what path we want to take here so i followed default behavior. Error message shown from declining a tx with a hardware wallet comes from the wallet itself, we'd have to some differentiate that contract data missing error from just a declined transaction. Thanks for the catch on the node switcher, fixed. |
* Fix unit dropdown alignment by rendering it in AmountField, and fixing a missed bootstrap case. * Fix modal amount and gas text. * Fix misaligned dropdown * Update conditions for NavLink is-active class
Changes:
The entire transaction stack is now implemented using redux/sagas/selectors instead of local component state.
The list of the components below have their functionality provided to them via dispatching an action on input change, where sagas will handle all of the business logic. Validation and data are all derived from selectors.
To integrate with the stack, the implementor can use the provided components located in
/common/components
:AmountField
Provides an unstyled input component that the user is able to enter in eitherether
ortoken
amounts, is green for valid inputs and red for invalid inputs. An invalid input occurs when the user has too low of a token balance (only online), too low of a ether balance (only online), not a number input, or when there are more decimal places than that token/ether supportsDataField
Provides an unstyled input component for the user to input data to be included in the transaction. Accepts both non hex and hex prefixed input. Interpreted as base16 (hex). Is invalid when the provided input is invalid hex.GasField
Provides an unstyled input component for gas limit for the user. Automatically displays updated gas limit whendata
orto
transaction fields change. If the network request fails, a fallback solution provided byethereumjs-tx
is used to still provide a rough estimate of what the gas limit will be. Estimations are debounced to 1 second. While a request is pending, the generate transaction button will be disabled. Allows manual input to gas limit which is validated for a valid float number.SendButton
Provides a button contains a transaction comparison box, offline send text, and online send button. When the user is using web3/metamask, the tx compare box displaystransaction parameters
/serialized transaction parameters
instead ofraw transaction
/sign transaction
. Reason being that the transaction displayed by web3/metamask is not yet signed, as it is signed and broadcasted in the same step. Button automatically switches between offline/online displaying depending if the client is offline/online. If online, the transaction will be broadcasted iff it is a new transaction, or a failed transaction. Otherwise it shows a warning notification that the transaction is already broadcasting or successfully broadcasted.SigningStatus
Provides a text component that prompts the user to sign their transaction on their hardware wallet provided that they have oneAddressField
Provides an unstyled input component forto
field input for the user. If left empty, the address is0x0
, which is the creation address. Validates against valid hex and checksummed addresses.AmountFieldFactory
A factory render callback component that lets the implementer choose their own input component and styling.ConfirmationModal
A confirmation modal component that gets used inSendButton
, pulls all required state from redux.DataFieldFactory
A factory render callback component that lets the implementer choose their own input component and styling.GasFieldFactory
A factory render callback component that lets the implementer choose their own input component and styling.GenerateTransaction
Provides an unstyled input component for signing a transaction. Disabled when the current state of the transaction does not match any of the transaction types outlined in Add link MyEtherWallet/MyEtherWallet#598. Disabled when a gas limit estimation is pending.NonceField
Provides an unstyled input component for displaying the users current nonce. Only renders if the client is inoffline
/forceOffline
state, or if the nonce fetch request failed. Allows for base 10 and base 16 input. Validates against invalid hex / number input.SendButtonFactory
A factory render callback component that lets the implementer choose their own input component and styling.SendEverything
Provides clickable text component that allows the user to send their entire balance of the currently selected unitUnitDropDown
Provides a styled dropdown that allows the user to select the unit they'd like to send. Upon re-selection, gas is re-estimated, and their balances are re-validated against the new balance of the selected unit. See theAmountField
component to see what will be revalidated.Non-standard transactions are now allowed to be done https://imgur.com/a/FS6EA. See here for the validation matrix implemented Add link MyEtherWallet/MyEtherWallet#598.
Broadcast transaction, Send transaction, Contract interact, and Contract Deploy all use the same transaction components and confirmation modal to keep all components that interact with transactions the same and avoid re-implementing the same features.
Transaction comparison, confirmation modal are added to web3/metamask transactions now
Error states for bad network / signing actions (for future use if we want to do more complex fallback actions based on failed requests)
Being able to switch between tokens without clearing fields + proper gas estimation
Clean up code base to only use 1-2 transaction interfaces instead of 4-6
Runtime validation of a formed transaction against the input fields is now done for both web3/metamask transactions and signed transactions. Signed transactions also have their signature validated.
Transaction state is cleared upon wallets being switched, transaction validation failure, and tab switching.
Old contract code is removed in favor of using the new contract library used in
Contract Interact
, ERC20 now uses this lib.Addresses:
Whats left for subsequent PRs
MyEtherWallet/MyEtherWallet#608