Skip to content
This repository was archived by the owner on Dec 21, 2021. It is now read-only.

Conversation

@timoxley
Copy link
Contributor

@timoxley timoxley commented Jan 18, 2021

This is basically a new streamr client with rebuilt internals, including a message subscription “pipeline”, more robust/understandable/consistent connection handling and a lot more tests + reliability around connection & timing issues.

External interface is largely the same, but timings may differ and there may be some edge cases not covered by tests.

This is built on top of, and supersedes changes in #156, #160, #162, #164 & #166

Rough WIP Changelog

  • subscribe/resend methods are now async.
  • No more subscribed event (subscribe() promise now resolves when this would have fired)
  • No more no_resend event, only resent whether there were messages or not.
  • Subscribe, resend & subscribe with resend all use the same machinery, so their behaviour should be identical.
  • Added collect() function to subscriptions that collects the results into an array of messages and resolves when the subscription ends, mainly/only useful for resends.
  • Subscriptions are async iterators. You can access this interface if you don’t supply an onMessage handler to subscribe/resend.
  • Connection emits done when it's been disconnected and will not reconnect.
  • Connection emits connected only when it's connected and will not be immediately disconnecting.
  • autoConnect + autoDisconnect options become forcibly disabled if you explicitly call connect() or disconnect(). This is because I could not come up a sensible, consistent expected behaviour that combined explicit & automatic connect/disconnect. Can reenable auto behaviour without creating a new client or mutating options using enableAutoConnect()/enableAutoDisconnect(). This means if you connect you must disconnect or enableAutoDisconnect() to close the connection.
  • connect() & disconnect() no longer error if already {dis}connecting/{dis}connected. This is basically what ensureConnected/ensureDisconnected provided before.
  • The ensureConnected/ensureDisconnected methods still exist but are now just aliases of connect & disconnect.
  • Uses latest version of streamr protocol 7.0.0 to get these fixes + more: Handle multiple gaps streamr-client-protocol-js#46 & Don't break if explicitly marking messages out of order. streamr-client-protocol-js#47
  • Random errors no longer disconnect the client.
  • Reconnecting on unexpected disconnection should work reliably now, there's a hard distinction between user-initiated disconnects and those that happen by accident.
  • Resends should work more reliably now.
  • Cleaned up and improved logging. Is more verbose but should be clearer what’s going on. todo: move logging to pino
  • Removed regenerator babel transform.
  • source-maps are now always on.
  • New BundleAnalyzerPlugin produces a bundle size report in dist/report.html. This is downloadable as an artifact in CI builds
  • Almost all functionality is now tested via integration tests, rather than implementation-coupled+heavily mocked unit tests.
  • Updated all tests to not use async (done) => in preparation for jest-circus jest runner being enabled by default in Jest 27+. Enabled jest-circus by default anyway. This fixes an issue where if a test fails asynchronously, the afterEach/afterAll hooks would run unchained i.e. beforeEach for next test would start before afterEach was complete. This led to some very confusing test results at times.
  • Moved data union tests into “flakey” test folder. These tests still run, but won’t fail CI.
  • Integrated new DU-2 changes
  • Significant improvements to CI workflow

Related: #NET-159

timoxley and others added 21 commits February 23, 2021 14:48
…improve validation, memberStats format (#206)

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
Type annotations for authFetch endpoints (and utils.test.ts).
createStream, getStream, getStreamByName and getOrCreateStream reject the promise if there is an API error.

To support this functionality, a new errorCode field was added to AuthFetchError. Currently the ErrorCode enum lists the most typical API error codes: VALIDATION_ERROR and NOT_FOUND. Other codes can be added later, if needed.
Types for getAdminAddress, setAdminFee, executeWithdraw. After these modifications, and PR #207 all public methods of DataUnion are annotated with valid types.

Remove deployTxReceipt field which was not used
Changed Connection.debug to optional parameter
Better error handling of authFetch errors: added classes ValidationError and NotFoundError.

Also smaller changes:
- added missing id field to StreamPermission
- strict type check of parameter in getOrCreateStream()
Export parameters/return types for objects which are publicly available to the JS-client users.

Updated getDataUnionMainnetAddress and getDataUnionSidechainAddress in Contracts.ts to support new DataUnion addresses.

BREAKING CHANGE:
- moved DataUnion related StreamrClient options into a new dataUnion block: factoryMainnetAddress, 
- factorySidechainAddress, minimumWithdrawTokenWei
- renamed payForSignatureTransport to freeWithdraw and reversed the value
- new required options in that block: templateMainnetAddress, templateSidechainAddress
timoxley and others added 6 commits March 11, 2021 10:25
Hide internal properties and methods from the public classes.

Currently the internals are annotated with a weak /** @internal */ . Some of the properties/methods could be marked with TypeScript's private to force the encapsulation. That can be done later, after we have converted all classes to TypeScript and the usage boundaries are clear.
Fetch the account address from MetaMask using request({ method: 'eth_requestAccounts' }). Previously used the deprecated selectedAddress field.

Configure chainId to StreamrClient options. The parameter is required when using MetaMask sidechain signing.

BREAKING CHANGE: client.getAddress() is now async

We could be more explicit about network chainIds. Sidechain chainId could be a required parameter in StreamrClient options and we could also add chainId parameter to options.mainnet definition. The chainId validation can be enhanced in another PR later, if needed.

Note that there are currently no automated tests to check the MetaMask integration.
Define valid types for public API methods

Also small bug fix for parsing stream definition options (e.g. when filtering subscriptions with getSubscription(opts))
@timoxley timoxley merged commit 8431431 into master Mar 12, 2021
@timoxley timoxley deleted the 5.x branch March 12, 2021 17:53
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.

3 participants