DataUnions: encapsulate contract handling, remove obsolete wrappers, improve validation, memberStats format

[teogeb]

Split DataUnionEndpoints.ts: move contract handling to Contracts.ts, ABI definitions to abi.ts and inline public methods to DataUnion.ts

Smaller changes:

• Remove obsolete caching from address fetching
• Improve address validation (and add tests)
• Split tests, improve test cleanup
• Bug fix for payForSignatureTransport option in withdraws: method option overrides client option
• Format of getMemberStats() data modified (enum values changed, number values string->BigNumber)
• Type annotations for StreamrClientOptions: many of the fields are required, as we apply a default value -> users give options as Partial<StreamrClientOptions>
• Id and debug are client properties, not options

[linear]

NET-205 JS-client: Simplify DataUnion interface

• Consistency to the method naming
• Hide internal DU-methods
• Get DataUnion by calling client.getDataUnion(address)
• Be more explicit about the addresses (remove "dataUnion" parameter from client config, do not default to the authenticated user on withdraw/getMemberStats/etc.)

[jtakalai]

stellar job

[jtakalai]

is id voluntary?

[teogeb]

Removed id and debug fields from options as those were duplicate to StreamrClient's fields. Neither of the fields affected the id and debug fields of the StreamrClient instance, so in practice those were not configuration options.

Changed Connection and WebSocket to get the debug instance from StreamrClient.

AuthFetch creates its own debug instance. If we later refactor it to a class, it can also get the debug instance from StreamrClient.

[timoxley]

👍 structure better. lgtm. Get @jtakalai approval before merging.

[timoxley]

What do you think about making passed-in client always the first parameter? For all my code that takes a client instance it's always first parameter, I think of it like a "pseudo-this" (this is a hidden implicit parameter to all functions, that's why function.call/apply take this as first parameter). Would make it consistent with existing code.

Also means if it takes a client instance, client is consistently in the same position without affecting arguments that normally come last e.g. options or callback.

[teogeb]

I usually put the parameters in priority order to improve readability. If the most important details are at the start of the line, the reader can easily spot what is happening at the line. The start of the line needs to be read anyway, but there is no need to read the line fully if the reader is just browsing the code to gather some high-level understanding.

If minor details (or highly predictable parameter values) are put the ends of line lines, the reader can focus to those later, if needed.

E.g. if someone reads only the bold parts in here, he/she should have quite a good understanding what the method does:

If (!foobar) {
const client = createClient()
doSomething(foo, bar, client)
somethingElse(foo, '123', options, client)
} else {
throw new Error()
}

Consistency is important, and the ability to curry functions. But here I'd like to trade those for the improved readability and "browseability".

If there are a group of methods that use "pseudo-this" parameters, we could refactor those to a class, like in Contracts.ts in this PR.

[jtakalai]

I'd say +1 to having the "self" as first argument. It could be a separate refactor though.

[timoxley]

Unrelated to this PR, but seems not ideal to me that getMainnetContract is sync, but getSidechainContract is async.

getSidechainContract is async because getSidechainSigner is async.

getSidechainSigner is only async because it calls async metamaskProvider.getNetwork to validate the network chainId:

// src/Ethereum.js
const { chainId } = await metamaskProvider.getNetwork()
if (chainId !== options.sidechain.chainId) {
    throw new Error(`Please connect Metamask to Ethereum blockchain with chainId ${options.sidechain.chainId}`)
}

I wonder if we could move this validation elsewhere, somewhere that's already async, so both getMainnetContract and getSidechainContract could be sync.

I'd almost be tempted to make getMainnetContract async just for consistency. cc @jtakalai

[teogeb]

@jtakalai suggested earlier that DataUnion could initialize the contract objects when it is created (e.g. getDataUnion()). In that case DataUnion would contain the actual Contract objects as fields, not contractAddress and sidechainAddress. Then we could fetch SidechainSigner in getDataUnion and provide the signer to getSidechainContract() as a parameter.

[jtakalai]

looking good! Please add here (or into the ticket) a list of links to tickets of refactorings that were not done at this time but could be done later.

[teogeb]

Ticket for possible refactorings: https://linear.app/streamr/issue/NET-209/js-client-refactoring-of-dataunion-internals