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

USDC related changes #151

Merged
merged 76 commits into from
Mar 20, 2024

Conversation

arthurgousset
Copy link
Contributor

@arthurgousset arthurgousset commented Feb 22, 2024

Description

A few sentences describing the overall effects and goals of the pull request's commits.
What is the current behavior, and what is the updated/expected behavior with this PR?

Other changes

Describe any minor or "drive-by" changes here.

Tested

An explanation of how the changes were tested or an explanation as to why they don't need to be.

Related issues

Backwards compatibility

Brief explanation of why these changes are/are not backwards compatible.

Documentation

The set of community facing docs that have been added/modified because of this change

ERC20 address should be whitelisted, but this is not checked or enforced.
Copy link

changeset-bot bot commented Feb 22, 2024

🦋 Changeset detected

Latest commit: d72868a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 20 packages
Name Type
@celo/contractkit Major
@celo/celocli Major
@celo/utils Patch
@celo/connect Patch
@celo/base Patch
@celo/explorer Patch
@celo/governance Patch
@celo/transactions-uri Patch
@celo/wallet-rpc Patch
@celo/cryptographic-utils Patch
@celo/keystores Patch
@celo/phone-utils Patch
@celo/wallet-base Patch
@celo/wallet-hsm-aws Patch
@celo/wallet-hsm-azure Patch
@celo/wallet-hsm-gcp Patch
@celo/wallet-ledger Patch
@celo/wallet-local Patch
@celo/wallet-remote Patch
@celo/wallet-hsm Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

- becomes synchronous call
- accepts `string` instead of `CeloTokenContract`
packages/cli/src/base.ts Outdated Show resolved Hide resolved
@arthurgousset
Copy link
Contributor Author

arthurgousset commented Feb 26, 2024

I'm checking which commands are affected by changes to the --gasCurrency flag

  1. config

For example:

$ celocli config:set --gasCurrency cUSD 

$ celocli config:get
node: https://forno.celo.org
gasCurrency: cUSD
  1. transfer
$ celocli transfer:dollars --from 0x303C22e6ef01CbA9d03259248863836CB91336D5 --to 0x22579CA45eE22E2E16dDF72D955D6cf4c767B0eF --value 1000000000000000 --gasCurrency cUSD --privateKey $PRIVATE_KEY --node alfajores

Running Checks:
   ✔  Account has at least 0.001 cUSD 
   ✔  Account can afford transfer and gas paid in cUSD 
All checks passed
SendTransaction: transfer
txHash: 0x6f69f6ac38c60afc6c66fb5f6d34531a8d59662ceaff1be0b0398c0533874f2b
Sending Transaction: transfer... done

Tip

Use this to convert --value from ether to wei in browser: eth-converter.com

  1. releasecelo

👉 Edit: Looks like most commands support the --gasCurrency flag (see comment below)

@arthurgousset
Copy link
Contributor Author

arthurgousset commented Feb 26, 2024

Out-of-scope for this PR

  1. The stated Nodejs version requirement (v18.14.2) seems wrong (it's 18.17.1 according to .nvmrc), which manifests in CLI README, NPM, and docs.celo.org

  2. releasecelo docs are available (https://docs.celo.org/cli/releasecelo), but are missing from the left-hand index on docs.celo.org. Logged here:

image

@arthurgousset
Copy link
Contributor Author

arthurgousset commented Feb 26, 2024

Noticed that --gasCurrency is a "base flag" defined with in BaseCommand class

export abstract class BaseCommand extends Command {
  static flags = {
    // ...
    gasCurrency: Flags.option({
      options: Object.keys(gasOptions),
      description:
        "Use a specific gas currency for transaction fees (defaults to 'auto' which uses whatever feeCurrency is available)",
      hidden: true,
    })(),
    // ...
  }

Source: cli > base.ts

The BaseCommand class is extended in various other classes like TransferStableBase for example.

export abstract class TransferStableBase extends BaseCommand {
  static flags = {
      ...BaseCommand.flags,
      // ...
  }
}

Source: cli > transfer-stable-base.ts

export default class TransferCelo extends BaseCommand {
  static flags = {
    ...BaseCommand.flags,
  // ...
  }
}

Source: cli > transfer > celo.ts

One way to find where --gasCurrency is supported is to do a code search for "...BaseCommand.flags".

@arthurgousset arthurgousset self-assigned this Feb 26, 2024
@arthurgousset
Copy link
Contributor Author

I have an approximate hunch what will need to be changed. I'll write some unit tests, before making changes across the CLI.

@arthurgousset
Copy link
Contributor Author

Nit re: StrongAddress = `$0x{string}` type

I'm not a fan of the variable name StrongAddress defined in @celo/base. I find StrongAddress is an odd name.

export type Address = string
export type StrongAddress = `0x${string}`

I'd prefer to use a variable called Address = `$0x{string}`

Probably not in scope to rename StrongAddress to Address in @celo/base at the moment.

@arthurgousset
Copy link
Contributor Author

arthurgousset commented Feb 26, 2024

I'm looking call the getWhitelist() function in the FeeCurrencyWhitelist.sol contract:

I notice there is no wrapper for this smart contract in ContractKit:

// [CeloContract.FeeCurrencyWhitelist]?: FeeCurrencyWhitelistWrapper,

Unlike the code example from the README suggests:

const feeCurrencyWhitelist = await kit._web3Contracts.getFeeCurrencyWhitelist()

It never existed and has been commented out since ContractKit was created in 2019 (see git blame)

I can always create a web3.eth.Contract instance by passing the ABI (available in ContractKit via @celo/abis) and contract address, but that doesn't seem like a good practice.

const contract = new web3.eth.Contract(feeCurrencyWhitelistABI, FEECURRENCYWHITELIST_CONTRACT_ADDRESS);

I have a feeling there must be a better way to do this. The contract is available via Web3ContractCache, but according to the docstring this is not how contracts are supposed to be used, instead I should use a contract wrapper.

/**
* Native Web3 contracts factory and cache.
*
* Exposes accessors to all `CeloContract` web3 contracts.
*
* Mostly a private cache, kit users would normally use
* a contract wrapper
*/
export class Web3ContractCache {

Edit: Nevermind, my bad, I found a way:

const feeCurrencyWhitelist = await kit._web3Contracts.getContract('FeeCurrencyWhitelist')
...  await feeCurrencyWhitelist.methods.getWhitelist()

@aaronmgdr
Copy link
Member

Nit re: StrongAddress = `$0x{string}` type

I'm not a fan of the variable name StrongAddress defined in @celo/base. I find StrongAddress is an odd name.

export type Address = string
export type StrongAddress = `0x${string}`

I'd prefer to use a variable called Address = `$0x{string}`

Probably not in scope to rename StrongAddress to Address in @celo/base at the moment.

I believe there is already an type called Address which is just set to string floating around so StrongAddress is to not confuse the two.

Strong as in Strongly Typed

@aaronmgdr aaronmgdr self-requested a review March 19, 2024 13:57
aaronmgdr
aaronmgdr previously approved these changes Mar 19, 2024
@aaronmgdr aaronmgdr self-requested a review March 19, 2024 14:01
@nicolasbrugneaux nicolasbrugneaux merged commit d6d82e8 into prerelease/mcrn Mar 20, 2024
13 checks passed
@nicolasbrugneaux nicolasbrugneaux deleted the arthurgousset/usdc-related-changes branch March 20, 2024 13:04
@github-actions github-actions bot mentioned this pull request Mar 20, 2024
nicolasbrugneaux added a commit that referenced this pull request Mar 20, 2024
* chore(connection): updates docstring for `defaultFeeCurrency`

ERC20 address should be whitelisted, but this is not checked or enforced.

* refactor(kit): updates `setFeeCurrency`

- becomes synchronous call
- accepts `string` instead of `CeloTokenContract`

* chore(setFeeCurrency): adds hex type and address validation

Does NOT validate it's a whitelisted fee currency, but asserts it's a valid hexadecimal address.

* nit(connect): adds feeCurrency type todo

* chore(kit): adds error handling in `setFeeCurrency`

I checked how errors are handled in other places.
For example: https://github.com/celo-org/developer-tooling/blob/319d00d54569b421dc994cb7848b7c96c1b524bf/packages/sdk/base/src/contacts.ts#L15

* nit(contrackit): adds TODO in README code example

* chore(kit): adds `throws` docstring in `setFeeCurrency`

For better developer experience

* nit(contrackit): adds another TODO in README code example

* nit(cli): adds TODO related to `setFeeCurrency`

* nit(connect): adds TODO related to hex address typing

* chore(kit): uses `isAddress` instead of `isHexString`

- web3js > `isAddress`
- @celo/base > `isHexString`

* chore(cli): automatic linting

* nit(CONTRIBUTING): fixes incorrect hyperlink

* chore(cli): adds TODOs re fee currencies

* chore(cli): adds pseudo-code to fetch fee currency whitelist on-chain

* chore(contractkit): adds TODO in README

* chore(cli): implements `getGasOptions` helper function

Untested, not sure this works as expected.

* chore(cli): adds TODO notes on `gasOptions`

* test(cli): adds test for `getGasOptions` helper function

* nit(README): fixes title

To be consistent with other READMEs, which use the package name as title

* test(helpers): fixes bug that caused test to fail

Passes as expected now.

* chore(cli): adds TODO

* chore(helpers): removes `return await`

In favour of `return ... as Promise<..>` (which is the same as `return await ...`)

Also renames the function for better readability

* chore(cli): adds TODOs and updates example

* chore(cli): adds incomplete/WIP changes

Committing my current work in progress, before handing over to Nico

* feat: change feeCurrency to be a StrongAddress is most places

* chore: regenerate docs

* chore: upgrade celo abis cr11

* fix: only throw if the flag has been passed and the value is undefined

* fix: celocli tests

* fix: add feecurency whitelisted currencies

* test: add wrong feeCurrency test

* test: add failing tests

* fix: gas estimation when submitting a tx

* fix: test

* fix: proper gas/value check

* refactor: test displays the faulty value

* chore: regenerate docs

* fix: rollback to celo-legacy tx

* chore: regenerate docs

* fix: revert some unwanted chaanges

* fix: remove CELO from the whitelisted currencies

* refactor: Address->StrongAddress

* fix: tests failing due to StrongAddress typings

* feat(BREAKING): change default behaviour of transfer commands

to defaults to eip1559 if no --gasCurrency flag was passed

* fix: StrongAddress

* fix: type

* chore: regenerate docs

* fix: getting gas balance

* fix: ElectionActivate tests

* chore: regenerate docs

* feat: add FeeCurrencyWhitelist wrapper and refactor kit and cli to use it

* await registerAccount() to pass all checks for "shows no pending votes" election:activate test

* chore: remove pasted comments

* chore: regenerate docs

* fix: gasCurrency fallback

* chore: remove comment

* feat: handle adapted token in the whitelist

* fix: prevent unecessary network call for every command

* fix: ts-expect-error directive

* chore: regenerate docs

* chore: add changeset

* fix: handle feeCurrency capitalization

* fix: handle compatibility issues with config and gasCurrency

* refactor: some PR review feedback addressed

* fix: handle CELO as a gasCurrency

* chore: add one changeset per package changed

* test: test adapted tokens too

* fix: handle CELO capitalization

---------

Co-authored-by: Nicolas Brugneaux <[email protected]>
Co-authored-by: Leszek Stachowski <[email protected]>
Co-authored-by: Leszek Stachowski <[email protected]>
@nicolasbrugneaux nicolasbrugneaux mentioned this pull request Mar 25, 2024
aaronmgdr pushed a commit that referenced this pull request Mar 26, 2024
- Remove stable token Inflation related methods and configs #186
- USDC related changes #151
- Added Ethereum-compatible legacy TXs support #182
- Update dependency cross-fetch to v3.1.5 [SECURITY] #168
- feat: ignores gasCurrency when passed to the config #198
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants