Skip to content
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

cEUR ContractKit support #7257

Merged
merged 34 commits into from
Mar 4, 2021
Merged

cEUR ContractKit support #7257

merged 34 commits into from
Mar 4, 2021

Conversation

tkporter
Copy link
Contributor

@tkporter tkporter commented Feb 24, 2021

Description

Adds StableTokenEUR and ExchangeEUR wrappers.

  • Adds Erc20Wrapper, which wraps the required functions of ERC20 tokens found in the IERC20 interface
  • Adds CeloTokenWrapper, which extends Erc20Wrapper and wraps functions in the ICeloToken interface. Although ICeloToken itself doesn't extend IERC20, it's intended to be used alongside IERC20, and having CeloTokenWrapper extend Erc20Wrapper is easier to get around TypeScript not supporting multiple inheritance (eg class StableTokenEURWrapper extends CeloTokenWrapper, Erc20Wrapper isn't possible)
  • StableTokenWrapper is used for both cUSD and cEUR.
  • ExchangeWrapper is used for both cUSD and cEUR. Dollar-specific functions still exist but are deprecated and the generic functions are preferred.
  • Created the class CeloTokens, a helper class for interacting with CELO & stable tokens

Other changes

  • Using a new version of @celo/typechain-target-web3-v1-celo to properly generate TS files for interfaces
  • Added stableTokenEUR to migration overrides for tests to work

I did not update packages/sdk/base/src/currencies.ts to include cEUR as only Valora relies upon that, and adding cEUR there caused a bunch of build issues that can only be resolved with code changes to support cEUR in Valora.

Tested

  • Ran tests
  • Ran against a testnet with cEUR

Related issues

Backwards compatibility

See Other Changes

@tkporter tkporter requested a review from a team February 24, 2021 13:58
@tkporter tkporter requested a review from a team as a code owner February 24, 2021 13:58
Copy link
Contributor

@gastonponti gastonponti left a comment

Choose a reason for hiding this comment

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

Ok, after reading it, I have a few questions/suggestions about the implementation.

Why don't we just create a StableTokenWrapper, that could handle every StableToken in it. Same with the exchange one.

In a way, the wrappers are things that we create to make everything easier. At least in my opinion, we shouldn't think the StableTokenWrapper as only the usd one, but all the stables.
To give you an example, the logic of searching for every balance could be part of the StableTokenWrapper.

Yes, today we have a wording problem, and the StableToken it means cUsd in our chain, but we could abstract the user from that.

What do you think about having a StableTokenWrapper, that basically switches between the stableTokens?

we could have something like:

const stable = kit.contracts.getStableToken()
stable.transfer(StableToken.cEUR, to, value)

And in that stable wrapper have something like:

transfer(st: StableToken, to: string, value: string | number)=> CeloTransactionObject<boolean> = proxySend(
    this.kit,
    this.contractMap[st].methods.transfer
  )

even we could overload the transfer to have the same but without the first parameter to asume that is cusd

transfer(to: string, value: string | number)=> CeloTransactionObject<boolean> = proxySend(
    this.kit,
    this.contractMap[StableToken.cUSD].methods.transfer
  )

AND in this same wrapper, for example, have the function that gathers all the stable token balances

We could maintain a wrapper per stable, but could be transparent for the user. The user knows that the StableTokenWrapper will handle everything, and internally we are the ones switching. And as is an internal wrapper, we could call the StableTokencUSDWrapper, the one that includes the generated StableToken
I think that this could make everything easier when we add a new stable, and will group the functions that collect info for every one of those (same with the exchange)

Don't know what you think about it, but it's adding a new layer to your actual work in this PR (nothing will be wasted, haha)

packages/protocol/scripts/build.ts Show resolved Hide resolved
packages/sdk/contractkit/src/contract-cache.ts Outdated Show resolved Hide resolved
packages/sdk/contractkit/src/kit.ts Outdated Show resolved Hide resolved
packages/sdk/contractkit/src/kit.ts Show resolved Hide resolved
@@ -117,6 +123,9 @@ export class WrapperCache {
getExchange() {
return this.getContract(CeloContract.Exchange)
}
getExchangeEUR() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as the StableToken comment

packages/sdk/contractkit/src/web3-contract-cache.ts Outdated Show resolved Hide resolved
packages/sdk/contractkit/src/web3-contract-cache.ts Outdated Show resolved Hide resolved
@tkporter
Copy link
Contributor Author

tkporter commented Feb 25, 2021

@gastonponti thanks for the review!
Re:

const stable = kit.contracts.getStableToken()
stable.transfer(StableToken.cEUR, to, value)

Personally I like the 1 wrapper instance : 1 contract relationship that (to my understanding) we've kept throughout contractkit. Because StableToken (cUSD) and StableTokenEUR (cEUR) are different contracts, I think it'd be confusing to have a single wrapper be able to reference both contracts. It'd probably involve a bigger refactor due to BaseWrapper having this.contract and things that only make sense for a single contract (like get address(): string). Additionally I think the ERC20 interface of things like transfer(to, value) are pretty standard and modifying that in contractkit to instead be transfer(token, to, value) feels a little weird and could cause devs to misinterpret the wrapper as referencing a single underlying contract that has a function transfer(token, to, value). Overall I think ContractKit should be a friendly wrapper for dealing with contracts as if they were nice TS objects, and it should generally try to expose functions to consumers directly, with small exceptions like calculating values that must be passed as a parameter correctly (thinking of SortedOracles calculating the lesser/greater for inserting here). Another reason I prefer transfer(to, value) relates to why I included the Erc20Wrapper-- we're already starting to see some small scale ERC20 tokens come to Celo, and it feels appropriate that people can interact with any ERC20 token via contractkit super easily if they want. Having the ability to use contractkit to transfer cUSD/cEUR/CELO/UBE/MOO/whatever with the same interface is super useful-- thinking about things like arb bots for Mento/Ubeswap, or future liquidation bots for Moola, etc

I do really like your suggestion of something like:

const stable = kit.contracts.getStableToken(StableToken.cEUR) // and have StableToken.cUSD be the default for backward compatibility
stable.transfer(to, value)

That way we keep the 1:1 wrapper/contract relationship, and as we add future stabletokens we don't need to add getStableTokenARS, etc. Do you have an opinion if this means we should keep the class StableTokenEURWrapper or if we should keep with the single class StableTokenWrapper and then have cUSD/cEUR/etc be instances of that wrapper? Personally the latter approach feels cleaner, but if we do that then I think it makes sense to do the same for ExchangeWrapper. There's the annoying bit with the Exchange having some USD-specific function names. We could possibly keep those as aliases but encourage people to use the functions using "stable" instead

@gastonponti
Copy link
Contributor

Agree. I think that we added a lot of methods in these two (stable and exchange) thinking that would be easier to read, instead of thinking about the better solution for N of those stables. I would keep it as aliases, and encourage to use the others, until we could eventually deprecate them.

Also agree on the Wrapper 1:1 but I also keep finding useful a way to handle all the stables at once. Maybe could be a helper or another class, that could keep your selected stable coins, and let you do stuff like show all the balances, or try to pay this X tx with one of these stables in a order [cUsd, cEur, cXXX] and send the tx from the first stable that could be cover the cost (I don't know, there are probably a lot of possible interactions between stables, and we should have something for that)
But I agree that shouldn't be what we use as the concept of wrapper

@gastonponti
Copy link
Contributor

And even knowing that this could be part of a different feature, I'm already seeing those kind of interactions that you added in the kit. So maybe extract it into some helper could be a really good first step

Copy link
Contributor

@gastonponti gastonponti left a comment

Choose a reason for hiding this comment

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

Nice work! So much clearer. Really like this approach.
I've added a little concern about the naming of a type, but is up to you 😄

LGTM

CELO = 'CELO',
}

export type CeloToken = StableToken | Token
Copy link
Contributor

Choose a reason for hiding this comment

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

(sorry missed to add the concern, haha)
Basically is the name collision between this type and the one in base (even that we know that the other will be deprecated). This could eventually bring some confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just changed to CeloTokenType

@tkporter tkporter added the automerge Have PR merge automatically when checks pass label Mar 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Have PR merge automatically when checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cEUR support for contractkit
2 participants