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

Conversation

@teogeb
Copy link
Contributor

@teogeb teogeb commented Feb 19, 2021

DataUnion is now an object which we can operate on. To get an instance of DU object, call client.getDataUnion(contractAddress). As the object encapsulates the contract address, we don't need to specify it when we access the DU methods.

Some methods that need a member address were previously optional, and the default value for those were the currently authenticated user. Now we need to be more explicit about the parameter and the member address is always required. Also previously there was a dataUnion client option which was a default value for some DU actions. That is no longer supported.

There is a new config option factorySidechainAddress. Users don't need to specify this as it has default value in test and will have a default value in production once we know the address.

Member join functions:

  • renamed joinDataUnion() -> join(secret?), if secret is given this will wait until the automatic join request has been processed
  • added isMember(memberAddress), returns true if the member is an active member
  • hasJoined() removed

Withdraw functions renamed to have word "all" if the amount is not specified:

  • withdrawAll(options?)
  • withdrawAllTo(recipient, options?)
  • signWithdrawAllTo(recipientAddress)
  • signWithdrawAmountTo(recipientAddress, amountTokenWei)
  • admin: withdrawFor() -> withdrawAllToMember(memberAddress, options?)
  • admin: withdrawAllToSigned(memberAddress, recipientAddress, signature, options?)
  • removed methods that returned the raw withdraw transactions removed: getWithdrawTx(), getWithdrawTxTo(), getWithdrawTxFor()

Public query functions:

  • renamed getDataUnionStats() -> getStats()
  • getMemberStats(memberAddress)
  • removed getMembers as it is not supported in DU v2
  • renamed getBalance() -> getWithdrawableEarnings(memberAddress)
  • getAdminFee()
  • getAdminAddress()
  • getVersion()

Admin functions:

  • client.deployDataUnion(options), waits until the deployment is ready, supports new options: confirmations and gasPrice
  • createSecret(name?)
  • addMembers(memberAddressList, options?)
  • renamed kick() -> removeMembers(memberAddressList, options?)
  • setAdminFee(newFeeFraction)

Other functions:

  • not related to DU: client.getTokenBalance()

Public accessors for DataUnion objects:

  • getAddress(), returns string
  • getSidechainAddress(), returns string

Internal functions:

  • client._getDataUnionFromName({ dataUnionName, deployerAddress }), returns DataUnion
  • dataUnion._getContract(), returns Promise

Future refactoring, should be done after this PR is in 5.x:

  • Move many DataUnionEndpoints.ts functions to DataUnion class (and inline DataUnion methods that just call an endpoint methods)
  • Move getTokenBalance to a separate class as it is not DU-related
  • Remove caching from fetchDataUnionMainnetAddress

… call

getMemberBalance -> getWithdrawableEarnings
move getTokenBalance
annotate tokenAddress as non-nullable in config as it has the default value
@linear
Copy link

linear bot commented Feb 19, 2021

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.)

@teogeb teogeb requested review from jtakalai and timoxley February 22, 2021 14:06
@teogeb teogeb changed the title WIP: NET-205: Simplify DataUnion interface NET-205: Simplify DataUnion interface Feb 22, 2021
Copy link
Contributor

@jtakalai jtakalai left a comment

Choose a reason for hiding this comment

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

DataUnion object is good stuff! So much better structure, used to be such a megafile amoeba.

Tests are passing but not exiting due to open handles. Just --forceExit them and make a ticket to track down the reason later. It can be really hard to find what it is, and it's probably inside ethers.js somewhere (despite doing await providerMainnet.removeAllListeners() in the end of tests, --detectOpenHandles isn't going to help much IIRC)

README.md Outdated

Data union functions take a third parameter, `options`, which are either overrides to options given in the constructor, or optional arguments to the function.

TODO: do we need these parameters (and `confirmations` parameter) somewhere? Either add the support, or remove from `DataUnionDeployOptions` interface (currently commented out)
Copy link
Contributor

Choose a reason for hiding this comment

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

yea could be a good thing I guess, goes together with gasPrice really. Leaving it out for now is no issue, can make a ticket to backlog to add it back later

},
"#IMPORTANT": "babel-runtime must be in dependencies, not devDependencies",
"dependencies": {
"@ethersproject/abi": "^5.0.12",
Copy link
Contributor

Choose a reason for hiding this comment

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

was this needed in the end?

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm looks like yes (abi.encode)

@teogeb teogeb merged commit 78449c1 into 5.x Feb 23, 2021
@teogeb teogeb deleted the NET-205-simplify-DU-interface branch February 23, 2021 13:58
Copy link
Contributor

@timoxley timoxley left a comment

Choose a reason for hiding this comment

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

lgtm

@timoxley timoxley mentioned this pull request Feb 23, 2021
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.

4 participants