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

finish polygon #2431

Merged
merged 19 commits into from
Sep 6, 2023
Merged

finish polygon #2431

merged 19 commits into from
Sep 6, 2023

Conversation

buck54321
Copy link
Member

Generalize the Ethereum wallet and server backend. Prepare getgas utility and RPC compatibility testing. Add tokens to Polygon harness and get gas estimates. Update some front end token-related code. Deploy USDC swap contract for Polygon mainnet and get gas estimates. Update simnet trade tests.

Copy link
Member

@chappjc chappjc left a comment

Choose a reason for hiding this comment

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

How do you use deploy? Where do you send the fee gwei? The deploy docs refer to "the wallet", but what wallet? Does this require a dexc assetdb wallet for the asset? I've used remix with web3 injection to deploy things as needed previously, so it's entirely independent.

Why is deploy.go in cleint/asset/eth instead of client/asset/eth/cmd/deploy? That seems wrong. If you want it reusable from tools other than cmd/deploy, maybe it should be in client/asset/eth/deploy. Seems like it's just polluting client/asset/eth like it is currently.

The deploy binary is checked in.

@buck54321
Copy link
Member Author

buck54321 commented Jul 10, 2023

How do you use deploy?

Instructions for using deploy are at the top of client/asset/eth/cmd/deploy/main.go.

Where do you send the fee gwei? The deploy docs refer to "the wallet", but what wallet? Does this require a dexc assetdb wallet for the asset?

See IMPORTANT: at the top of deploy/main.go. deploy uses the same wallet credentials as getgas. It offers similar auxiliary functions to getgas for estimating funding requirements and returning balance.

I've used remix with web3 injection to deploy things as needed previously, so it's entirely independent.

What's the benefit of it being entirely independent? Is "web3 injection" just a webpage using the provider API by bouncing through a wallet extension? That's what is seems to say here.

Does Remix offer any advantages? I'm not sure how etherscan gets the metadata associated with the contract. For instance, here is the ethereum mainnet swap contract. How do they get the source code and ABI info? Does Remix do that?

Why is deploy.go in cleint/asset/eth instead of client/asset/eth/cmd/deploy?

I can move it if you'd rather I export some stuff.

@@ -8,7 +8,7 @@ If this is a new asset, you must populate either `dexeth.VersionedGases` or
`dexeth.Tokens` with generous estimates before running `getgas`.

### Use
- Create a credentials file. `getgas` will look in `~/ethtest/getgas-credentials.json`. You can override that location with the `--creds` CLI argument. The credentials file should have the JSON format in the example below. The seed can be anything.
- Create a credentials file. `getgas` will look in `~/ethtest/getgas-credentials.json` for Ethereum, and `~/ethtest/getgas-credentials_polygon.json` for Polygon. You can override that location with the `--creds` CLI argument. The credentials file should have the JSON format in the example below. The seed can be anything.
Copy link
Member Author

Choose a reason for hiding this comment

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

I actually want to combine these into one file and move everything from ethtest somewhere under dextest.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK if I do this now and screw up all of your credentials files?

dex/networks/polygon/params.go Show resolved Hide resolved
dex/testing/polygon/harness.sh Show resolved Hide resolved
sed -i.tmp "s/TEST_TOKEN=.*/TEST_TOKEN=\"${TEST_TOKEN_BYTECODE}\"/" ../../../testing/eth/harness.sh
# mac needs a temp file specified above.
rm ../../../testing/eth/harness.sh.tmp
for HARNESS_PATH in "$(realpath ../../../testing/eth/harness.sh)" "$(realpath ../../../testing/polygon/harness.sh)"; do
Copy link
Member Author

Choose a reason for hiding this comment

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

Not super crazy about this solution. This pastes the exact same byte code into both harnesses, so perhaps some room for de-duplication.

@chappjc
Copy link
Member

chappjc commented Jul 10, 2023

Why is deploy.go in cleint/asset/eth instead of client/asset/eth/cmd/deploy?

I can move it if you'd rather I export some stuff.

Don't bother. I'd rather have some new tangential stuff in client/asset/eth (ContractDeployer) than unnecessary exports.

Everything is fine, no need to change anything. For the sake of discussion though:

How do you use deploy?

Instructions for using deploy are at the top of client/asset/eth/cmd/deploy/main.go.

I had read that entire block comment, but didn't read into get gas. Sorry.

I've used remix with web3 injection to deploy things as needed previously, so it's entirely independent.

What's the benefit of it being entirely independent?

I mainly mentioned that approach to explain why the proposed tool gave me pause. However, reasons why I personally prefer an approach for contract deployment that is separate:

(1) It's already available. We don't have to make the package modifications and trade-offs above. (2) It's interactive from compilation to deployment. At a glance, I can't tell what compiled runtime bytecode deploy/main.go is going to deploy. (It's the Bin in the pre-generated contract.go). When I deploy a contract interactively, I be sure to set the solc version and optimization iterations, make sure it didn't have any warnings etc., enter the target erc20 address, set and sanity-check the fees. (3) I use the wallet of my choice to deploy (something with no other purpose than being a "deployer", although I now see that getgas-credentials.json makes it somewhat separate although likely to be derived from a dex wallet seed)

Does Remix offer any advantages? I'm not sure how etherscan gets the metadata associated with the contract. For instance, here is the ethereum mainnet swap contract. How do they get the source code and ABI info? Does Remix do that?

You upload the source to etherscan manually. Remix isn't related to that from what I recall. When you upload, you specify the same solc build config (version and opt iterations) used when building the contract's bin.

Copy link
Contributor

@martonp martonp left a comment

Choose a reason for hiding this comment

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

Regarding deployment, why don't we just use hardhat?
I've used it in #2223 to do testing. It's nice because it allows you to directly test the smart contract without worrying about anything else. You can also deploy with it.
https://hardhat.org/

client/asset/polygon/polygon.go Outdated Show resolved Hide resolved
dex/testing/polygon/harness.sh Show resolved Hide resolved
ctx, cancel := context.WithTimeout(ctx, defaultRequestTimeout)
err := t.f(ctx, p)
cancel()
if err != nil {
return fmt.Errorf("RPC Provider @ %q has a non-compliant API: %v", p.host, err)
return fmt.Errorf("%s: RPC Provider @ %q has a non-compliant API: %v", t.name, p.host, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

I see this error log in the node after the compatibility test.. I guess the connection is not being closed properly after the tests. Probably nbd though.

Screenshot 2023-07-11 at 5 36 42 AM

Copy link
Member Author

Choose a reason for hiding this comment

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

What were you running when you saw this message? dexc or a test?

Copy link
Contributor

Choose a reason for hiding this comment

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

dexc

client/asset/eth/eth.go Outdated Show resolved Hide resolved
@chappjc
Copy link
Member

chappjc commented Jul 19, 2023

Any suggested Polygon RPC providers? Every single one at https://github.com/decred/dcrdex/wiki/Ethereum#rpc-provider-list-partial have Polygon endpoints, but I wondered if you has success with any.

@chappjc
Copy link
Member

chappjc commented Jul 19, 2023

https://rpc.ankr.com/polygon_mumbai is probably fine although no ws
https://polygon-mumbai-bor.publicnode.com too

@buck54321
Copy link
Member Author

Alchemy is definitely working. I think I was having problems with Infura and Rivet. I need to do some testing with those.

Copy link
Member

@chappjc chappjc left a comment

Choose a reason for hiding this comment

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

Basic wallet functionality for MATIC and USDC worked well after enabling usdc.polygon (see comments and diffs).

Tested Mumbai (testnet) wallet with blast and alchemy endpoints w/ api keys, but here are some public endpoints that could work:

https://polygon-mumbai-bor.publicnode.com
https://polygon-mumbai.blockpi.network/v1/rpc/public
https://rpc-mumbai.polygon.technology
https://endpoints.omniatech.io/v1/matic/mumbai/public
wss://polygon-testnet.public.blastapi.io
https://rpc.ankr.com/polygon_mumbai

image

image
(fine, although above it doesn't mention polygon or overlay the logo; an overlay might be too small anyway)

image

image

I configured MATIC on dex-test.ssgen.io (testnet), but did not test any trades with MATIC, only setup the endpoints and confirmed it would start. Next, I will test a MATIC trade, and then enable USDC.POLYGON on server.

I used these on the testnet server (only the first needs an apikey):

wss://polygon-mumbai.blockpi.network/v1/ws/<apikey>
wss://polygon-testnet.public.blastapi.io
https://polygon-mumbai.blockpi.network/v1/rpc/public
https://polygon-mumbai-bor.publicnode.com
https://rpc.ankr.com/polygon_mumbai

client/asset/polygon/polygon.go Outdated Show resolved Hide resolved
server/asset/eth/eth.go Show resolved Hide resolved
server/asset/polygon/polygon.go Outdated Show resolved Hide resolved
server/asset/polygon/polygon.go Show resolved Hide resolved
@chappjc
Copy link
Member

chappjc commented Jul 21, 2023

I suppose the following should say "MATIC" instead of "POLYGON":

image

Settled fine!

image

@chappjc
Copy link
Member

chappjc commented Jul 21, 2023

Another MATIC vs. POLYGON that I noticed:

image
(right)

image
(wrong)

Probably also this:
image

Copy link
Member

@JoeGruffins JoeGruffins left a comment

Choose a reason for hiding this comment

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

Working well for me.

One thing I noticed on order history, symbols not there, does it look like this for others?

image

Comment on lines +407 to 411

# DRAFT NOTE: Why are we doing this token send to nowhere?
TOKEN_SEND_AMT=5000
echo "Sending 5000 dextt.eth to testing."
"./sendTokens" "${SIMNET_TOKEN_ADDRESS}" "${TOKEN_SEND_AMT}"
Copy link
Member

Choose a reason for hiding this comment

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

Probably the simnet tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can't find a reference to the SIMNET_TOKEN_ADDRESS anywhere.

Copy link
Member

@JoeGruffins JoeGruffins Jul 24, 2023

Choose a reason for hiding this comment

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

It's the address made by the seed in the ./simnet-setup.sh script so we have funds for trading immediately.

image

echo "ERC20 SWAP contract address is ${ERC20_SWAP_CONTRACT_ADDR}. Saving to ${NODES_ROOT}/erc20_swap_contract_address.txt"
cat > "${NODES_ROOT}/erc20_swap_contract_address.txt" <<EOF
${ERC20_SWAP_CONTRACT_ADDR}
EOF

Copy link
Member

Choose a reason for hiding this comment

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

You commented that mining is wonky is that why no 15 second mining session? If possible can add like the other harnesses?

Copy link
Member Author

Choose a reason for hiding this comment

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

Trying it out. Reduced min NUM for mine-alpha script to 1 too, So hopefully that helps.

client/asset/eth/deploy.go Outdated Show resolved Hide resolved
client/asset/eth/deploy.go Outdated Show resolved Hide resolved
client/asset/eth/deploy.go Outdated Show resolved Hide resolved
@chappjc
Copy link
Member

chappjc commented Jul 22, 2023

One thing I noticed on order history, symbols not there, does it look like this for others?

Same here. All the symbols are missing.

In the browser console I see 404s to the generic coin letter images, also upper case, which breaks their loading although we don't want the generic ones anyway:

image

@@ -479,7 +479,7 @@ var bipIDs = map[uint32]string{
890: "xsel",
900: "lmo",
916: "meta",
966: "matic",
966: "polygon", // changed from "matic"
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should keep it here as "matic", since our bipIDs maps ID to symbol, not coin name.

This npm package addressed it by mapping to both symbol and coin name. https://github.com/bitcoinjs/bip44-constants#readme
Although I suppose it doesn't really matter if we just use the correct fields from UnitInfo...

Copy link
Member Author

@buck54321 buck54321 Jul 22, 2023

Choose a reason for hiding this comment

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

The problems we're seeing are all places where we we're using symbol and we should have been using conventional unit. I like this change particularly because it brings those mistakes to the surface.

@buck54321 buck54321 force-pushed the more-polygon branch 2 times, most recently from 606116a to cfe1998 Compare July 23, 2023 02:57
@norwnd
Copy link
Contributor

norwnd commented Jul 23, 2023

Perhaps for token deposit/send views it would be nice to display which Blockchain was chosen (when user clicked deposit/send button), so that user doesn't accidentally send from Ethereum blockchain to Polygon (or vice versa):

image image

Another thing to consider: currently I'm guessing Polygon and Ethereum private keys are different resulting in different addresses for each; while it's better from security perspective, is it really worth it ? Pretty much all exchanges I can remember give the user that impression that addresses across "similar" blockchains match.

@buck54321
Copy link
Member Author

buck54321 commented Jul 25, 2023

NOTICES

1) For anyone who has used getgas, deploy, or anyone that has run nodeclient_harness_test.go on testnet

I have consolidated all of these credientials-file-based wallets into a single file at ~/dextest/credentials.json. See the getgas README for the format. The ~/ethtest directory is no longer used for anything, nor are the ~/ethtest/testnet-credentials.json, ~/ethtest/getgas-credentials.json, ~/ethtest/getgas-credentials_polygon.json, ~/ethtest/providers.json, or ~/ethtest/providers_polygon.json files.

Sorry if this causes some work for you, but the old scheme was getting out of hand.

2) For anyone who has sent funds to a testnet or mainnet Polygon wallet

As per @norwnd's suggestion, I have made changes such that all account-based assets will use the same seed, so will derive the same key and address.

Copy link
Contributor

@martonp martonp left a comment

Choose a reason for hiding this comment

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

Tested trading on both Infura and Alchemy.. working well.
One time on Infura I saw some crazy amount of confirmations:
Screenshot 2023-08-08 at 6 17 52 AM

it stagnated a bit and there was an error:

2023-08-08 10:17:44.487 [ERR] CORE: dex-test.ssgen.io:7232 tick: {redeemMatches order 85593c2ba1401832961278403497233287980a1cdd519dcb3fe17432095fb6cd - {error sending redeem transaction: error getting balance in excessive gas fee recovery: pending balance error: balance undeflow detected. pending out (134900000000000000) > balance (61809836994177172)}}

Then it eventually resolved itself. Looks like a uint32 underflow on the confirmations. It worked fine every other time.

On the wallets page the icons look like this.
Screenshot 2023-08-08 at 5 26 14 AM
It would be better if this was sorted so that the base chain comes first, and then all of its tokens listed below. I think ideally only the token would be listed on the sidebar and then within the token section you would see each of the networks it is on and your balance on each network.

Also, I see "polygon" written in some places and "matic" in others. I think it should be matic everywhere other than the "Token on Polygon" message.

client/asset/eth/chaincfg.go Outdated Show resolved Hide resolved
client/asset/polygon/polygon.go Outdated Show resolved Hide resolved
client/core/core.go Show resolved Hide resolved
@buck54321
Copy link
Member Author

It would be better if this was sorted so that the base chain comes first, and then all of its tokens listed below. I think ideally only the token would be listed on the sidebar and then within the token section you would see each of the networks it is on and your balance on each network.

I agree.. Mind if I save it for a separate, dedicated effort.

@martonp
Copy link
Contributor

martonp commented Aug 22, 2023

I still see polygon instead of matic in the following places.

Screenshot 2023-08-22 at 8 00 48 AM Screenshot 2023-08-22 at 8 00 24 AM Screenshot 2023-08-22 at 8 00 17 AM Screenshot 2023-08-22 at 8 00 03 AM Screenshot 2023-08-22 at 7 59 36 AM

Also, token icons are messed up on the orders page:

Screenshot 2023-08-22 at 8 09 11 AM

@martonp
Copy link
Contributor

martonp commented Aug 22, 2023

Overall LGTM.. I'll test a bit more still on testnet.

@martonp
Copy link
Contributor

martonp commented Aug 27, 2023

I'm able to do both matic and polygon usdc swaps on testnet. On the orders page, the block explorer links for polygon token transactions do not work.

@buck54321 buck54321 force-pushed the more-polygon branch 2 times, most recently from 49c1a14 to c179cc0 Compare August 28, 2023 01:03
@buck54321
Copy link
Member Author

Updated the free server list and upped the defaultGasFeeLimit. Tested on mainnet.

@buck54321 buck54321 merged commit 3fe9316 into decred:master Sep 6, 2023
5 checks passed
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.

6 participants