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

(fix) naming get_connector_spec fix #6992

Open
wants to merge 9 commits into
base: development
Choose a base branch
from

Conversation

isreallee82
Copy link
Contributor

@isreallee82 isreallee82 commented Apr 26, 2024

Before submitting this PR, please make sure:

  • Your code builds clean without any errors or warnings
  • You are using approved title ("feat/", "fix/", "docs/", "refactor/")

A description of the changes proposed in the pull request:

Tests performed by the developer:

while working on hummingbot/gateway#316 noticed
curl tests work fine with balance and allowances
but I get error from the client

ValueError: not enough values to unpack (expected 2, got 1)

Screenshot 2024-04-26 at 1 51 22 PM

Due to naming convention client uses avalanche i.e Network as chain, making the split process not happen leading to a bug

Screenshot 2024-04-26 at 1 58 05 PM

Tips for QA testing:

@isreallee82
Copy link
Contributor Author

cc @nikspz @fengtality

@fengtality
Copy link
Sponsor Contributor

From the code in this PR, you are just changing the name of the locally scoped chain variable to chain_name. That should have any impact outside of naming and I don't see how it can fix an error like ValueError: not enough values to unpack (expected 2, got 1) where it's related to arguments not being there or something like that.

@isreallee82
Copy link
Contributor Author

isreallee82 commented May 4, 2024

updated:

@fengtality @nikspz after alitlle digging the iteration must have stopped on avalanche from SUPPORTED_CHAINS set, so its final result must have been Uniswap_avalanche_avalanche,

so I figured the reason why chain_name worked instead of chain was there was changes to arrangements in the set SUPPORTED_CHAINS which placed ethereum first on the iteration list

For chain
Screenshot 2024-05-04 at 1 34 28 PM

For chain_name
Screenshot 2024-05-04 at 1 36 31 PM

Either ways both solutions were buggy

I figured another solution on my latest commit
supported chains have no underscore I came up with this solution.
I am opened to corrections @fengtality

@isreallee82
Copy link
Contributor Author

I think this approach is more simple and solves the problem @nikspz

@fengtality
Copy link
Sponsor Contributor

I think this approach is more simple and solves the problem @nikspz

Agreed!

Copy link
Contributor

@nikspz nikspz left a comment

Choose a reason for hiding this comment

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

Test performed:

  • Cloned and installed PR6992 and gateway latest development
  • connected gateway uniswap_ethereum_arbitrum, pangolin_avalanche_avalanche, uniswapLP_ethereum_arbitrum, sushiswap_ethereum_mainnet, sushiswap_avalanche_avalanche, quickswap_polygon_mainnet
  • checked balance successfully

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: On Deck
Development

Successfully merging this pull request may close these issues.

None yet

3 participants