Dev Documentation and Async Only Validators in Ethereum Chain Adapter

[GMSteuart]

I will start by saying I am going to keep this discussion short since there is a pull request with part of the affected code.

Current implementation has synchronously implemented methods defined as synchronous. The reason I am told for this decision is for consistency and flexibility. Restricting the development team to one implementation type is anything but flexible. Resolving issues all the same way is inconsistent problem solving. I say treat different problems differently.

Pull request in question: shapeshift/lib#517

I would like to suggest the following:

• Development decisions be documented in the code. Preferably with a link to an issue, discussion, etc.
• Update the affected code in the pull request above to be synchronous

I would like more information on the following:

• Documentation on how this development decision has affected the design

Please feel free to ask anything or make additional statements. I will provide clarity and answer any questions I can to the best of my abilities.

[mrnerdhair]

We're not restricting implementers to using asynchronous implementations, we're restricting consumers from counting on every implementation always being synchronous. That does make frontend work a bit harder, but it keeps the lower layers flexible.

While it's true that no current chain adapter implements this method in a way that requires async processing, I could easily imagine some use cases that would be completely impossible if synchrony were assumed at the API level:

• Extending e.g. validateAddress()/validateEnsAddress() to check that the address actually exists on-chain, not just that it's formatted correctly
• Offloading a chain adapter to a web worker so expensive crypto operations don't block the API
• Running a chain adapter in an isolated origin to protect it from XSS
• Building a chain adapter in WASM -- that could be a possible way to sandbox plugin code which adds new chain support, or achieve compatibility with something like Substrate

IMO, making the assumption that no API implementation will ever need any data that's not available during the execution of the current microtask is short-sighted and not worth the tradeoff for making frontend code a bit easier. If dealing with Promises in React is painful, we need to do some research or make some tooling to eliminate that pain at the source, not push the problem upstream.

[0xdef1cafe]

@mrnerdhair these concerns are largely academic and premature optimization IMO. i agree with @GMSteuart and will approve these changes to make unnecessarily asynchronous methods synchronous as a codeowner.

[mrnerdhair]

We've learned the hard way in hdwallet that API choices can last a long, long time. Using asynchronous methods and promises is a fairly standard API design these days; I don't think I've seen a new web standard released in years that returns synchronously, even if the common implementations would permit it. You may consider these considerations academic, but I have the distinct feeling that requiring significant portions of the UI code to be rewritten in order to deliver a feature like origin isolation for a chain adapter will reduce the chance that it can ever happen.

We're in a DAO, and this is an open-source project. Code ownership is intended to to be a position of stewardship. Changes like this should involve consideration and consensus-building and not resolution by administrative fiat.