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

[VEN-2299]: add a contract to operate token converters #3

Merged
merged 5 commits into from
Jan 25, 2024

Conversation

kkirka
Copy link
Contributor

@kkirka kkirka commented Dec 20, 2023

No description provided.

@kkirka kkirka changed the base branch from main to feat/move-debt-operator December 20, 2023 13:30
@kkirka kkirka force-pushed the feat/converter-operator branch from ed00401 to 93b043e Compare December 22, 2023 11:38
@chechu chechu changed the title feat: add a contract to operate token converters [VEN-2299]: add a contract to operate token converters Jan 11, 2024
@kkirka kkirka force-pushed the feat/converter-operator branch from cd0dd1b to 08512c8 Compare January 18, 2024 10:53
package.json Outdated
"@venusprotocol/protocol-reserve": "1.0.0-converters.1",
"@venusprotocol/venus-protocol": "6.1.0-dev.8"
"@venusprotocol/protocol-reserve": "1.3.0-dev.1",
"@venusprotocol/venus-protocol": "6.1.0-dev.8",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use release versions?
@venusprotocol/protocol-reserve - 1.2.0
@venusprotocol/venus-protocol - 7.0.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure about protocol-reserve, but we can definitely use core v7. I'll update the other ones as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -0,0 +1,227 @@
// @kkirka: This is a typescript file and not a JSON file because I wanted to keep
Copy link
Contributor

Choose a reason for hiding this comment

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

I would recommend using typechain and getting abis from packages

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Typechain devs say it's going to die at some point: https://github.com/dethcrypto/TypeChain?tab=readme-ov-file#soft-deprecation-notice, and I'm not sure typechain types are compatible with viem.

Actually this file is just export default <ABI JSON> as const;, so it should be straightforward to import ABIs from packages and "decorate" them this way.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, you can't create a const from imported JSON. It looks like HasAddressFor isn't working as expected for this reason.

I've started working on a postinstall step where we generate EIP-712 using the wagmi/cli hardhat plugin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Weird, HasAddressFor worked for me because importing JSON yields a pretty narrow type when JSON is an object (not as narrow as const but still). Maybe it failed because governance-contracts was missing? I've added it here: 77064f9

image

Still, wagmi/cli sounds good, as long as you're ok with using viem

import bsctestnetCore from "@venusprotocol/venus-protocol/deployments/bsctestnet_addresses.json";

export const addresses = {
// TODO: Replace hardcoded addresses with addresses from the packages
Copy link
Contributor

@coreyar coreyar Jan 18, 2024

Choose a reason for hiding this comment

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

Is something missing to complete this todo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the PR with the converters isn't merged yet, even to dev: VenusProtocol/protocol-reserve#9

Copy link
Contributor Author

Choose a reason for hiding this comment

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

readonly hex: Hex;
}

export const parsePath = (data: Array<Address | number>): Path => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: An idea to make this safer would be accept an input like Array<[address, number, address]>. Since each part of the path will have the same 3 parts

Copy link
Contributor Author

@kkirka kkirka Jan 18, 2024

Choose a reason for hiding this comment

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

Yeah, I didn't like Address | number either as it doesn't really convey the shape of the expected input.

The path goes like:
[USDT, 0.5%, WBNB, 0.3%, BTC, 0.1%, XVS]

Do you propose to instead use this kind of path:
[[USDT, 0.5%, WBNB], [WBNB, 0,3%, BTC], [BTC, 0.1%, XVS]]?

It seemed too verbose to me at first, but now I think that maybe it's more readable 🤔

}

get publicClient() {
return this._publicClient ?? getPublicClient(this.chainName);
Copy link
Contributor

Choose a reason for hiding this comment

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

this._publicClient never receives an assignment. Why not set this in the constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oups, I wanted some sort of laziness here, but actually did it wrong, lol 🤦 😅

The intent was sth like

get publicClient() {
  if (!this._publicClient) {
    this._publicClient = getPublicClient(this.chainName);
  }
  return this._publicClient;
}

Copy link
Contributor Author

@kkirka kkirka Jan 18, 2024

Choose a reason for hiding this comment

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

Maybe there are better ways to lazily initialize things? Some lib or a common approach (memoization, whatever)

Copy link
Contributor

Choose a reason for hiding this comment

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

You could use the logical or assignment operator ||=

Copy link
Contributor Author

Choose a reason for hiding this comment

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

}

get walletClient() {
return this._walletClient ?? getWalletClient(this.chainName);
Copy link
Contributor

Choose a reason for hiding this comment

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

this._walletClient never receives an assignment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, same thing as with public client

Copy link
Contributor Author

Choose a reason for hiding this comment

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

const beneficiary = this.walletClient.account.address;
const chain = chains[this.chainName];

if (minIncome < 0n) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would minIncome be negative?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sometimes we're willing to pay for conversions to happen, even if it's not profitable. Actually in the initial phase the incentives would be zero, so we'll probably be paying for all conversions.

): WalletClient<HttpTransport, typeof chains[ChainT], PrivateKeyAccount> => {
return createWalletClient({
chain: chains[chainName],
transport: http(process.env[`LIVE_NETWORK_${chainName}`]),
Copy link
Contributor

Choose a reason for hiding this comment

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

This env var is missing from the example

};

const readPrivateKeyFromEnv = (chainName: string): PrivateKeyAccount => {
const key = process.env[`PRIVATE_KEY_${chainName}`];
Copy link
Contributor

Choose a reason for hiding this comment

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

This env var is missing from the example

@kkirka kkirka force-pushed the feat/converter-operator branch from 08512c8 to 742773f Compare January 25, 2024 14:05
@kkirka kkirka changed the base branch from feat/move-debt-operator to main January 25, 2024 14:06
@kkirka kkirka force-pushed the feat/converter-operator branch from a60912c to 77064f9 Compare January 25, 2024 14:17
@kkirka kkirka force-pushed the feat/converter-operator branch from 77064f9 to 3418642 Compare January 25, 2024 14:25
@kkirka kkirka merged commit b3d3c3f into main Jan 25, 2024
@kkirka kkirka deleted the feat/converter-operator branch January 25, 2024 14:26
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.

2 participants