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

feat: primary pool downscale and upscale new logic #3

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Mohzcrea8me
Copy link
Collaborator

@Mohzcrea8me Mohzcrea8me commented Oct 10, 2023

Hi, i worked on paraswap integration with verified, and SOR repo was given to me as part of the resources to use along side with primary and secondary pool contracts by @kallolborah . SOR code was helpful in helping me understand how the pool works but i noticed slight difference in upscale and downscale logic used for primarypools i tried calculating manually it does not match up too.

I'm creating this PR so the developer can review the new logic i added to primary pool, i think it's in accordance with the primaryIssuepool contract. I may be wrong would love a feedback from the developer.

Note: i'm unable to run tests i tried the method from readme it exited with errors.The developer/reviewer should let me know how to run tests. Thanks

@Parasgr7
Copy link

Parasgr7 commented Oct 10, 2023

Hey @Mohzcrea8me , To run a specific test file in SOR, the test command is mentioned at the top of the specific file.
& Code is even mentioned in the package.json file in the scripts section.
I.e For example to run a poolsPrimary test file
Run: TS_NODE_PROJECT='tsconfig.testing.json' npx mocha -r ts-node/register test/poolsPrimary.spec.ts

@Parasgr7
Copy link

@Mohzcrea8me
Try and recheck your code by running the test cases. It throws some error
SyntaxError: Cannot convert 5192296858532827.628530496329220095 to a BigInt

@Mohzcrea8me
Copy link
Collaborator Author

Hey @Mohzcrea8me , To run a specific test file in SOR, the test command is mentioned at the top of the specific file. & Code is even mentioned in the package.json file in the scripts section. I.e For example to run a poolsPrimary test file Run: TS_NODE_PROJECT='tsconfig.testing.json' npx mocha -r ts-node/register test/poolsPrimary.spec.ts

okay, thanks testing it out now

@Mohzcrea8me
Copy link
Collaborator Author

@Mohzcrea8me Try and recheck your code by running the test cases. It throws some error SyntaxError: Cannot convert 5192296858532827.628530496329220095 to a BigInt

i will checkout the test cases and handle it, i changed the allBalanceScaled to bigint i think that's where it's coming from

@Mohzcrea8me
Copy link
Collaborator Author

Hey @Mohzcrea8me , To run a specific test file in SOR, the test command is mentioned at the top of the specific file. & Code is even mentioned in the package.json file in the scripts section. I.e For example to run a poolsPrimary test file Run: TS_NODE_PROJECT='tsconfig.testing.json' npx mocha -r ts-node/register test/poolsPrimary.spec.ts

okay, thanks testing it out now

This worked thanks, can you review my PR @kallolborah @Parasgr7

@Parasgr7
Copy link

@Mohzcrea8me from where have you taken this data added in file primaryPool.json?
I don't think it is coming from the subgraph.

There is no scaling(decimal fixed with 18 or 6 digits) in balances present on the subgraph.
Check below the actual data coming from the subgraph.

Screenshot 2023-10-11 at 6 12 50 PM

I would like you to revisit your test cases and the [upscaling/downscaling]changes you have made in primary pool, I don't think those are correct.

@Mohzcrea8me
Copy link
Collaborator Author

The data is from polygon subgraph I'm using for paraswap, exact tokens and balances for primaryissuepool.

I don't understand what you meant by no decimal scaling on subgraph, I was using sor as reference while working on paraswap I have access to primary pool contract as well and I noticed the calculations are the same but the scaling and downscaling method differs. I will attach screenshots to further explain what I'm talking about

@Parasgr7
Copy link

I went through your code @Mohzcrea8me , few things l would like you to change

  1. Pair of test cases: _tokenInForExactTokenOut, calculations is not correct. Please go through the test cases once which I have done in branch pool_test.
  2. What is the reason for removing parsePoolPairData, limit amounts, _spotPriceAfterSwap test cases?
  3. Remove unnecessary comments, and please do proper code formatting.
  • Define variables for address[Currency/Security] to be used in test file
  • Optimise the primaryIssuePool code also

@kallolborah
Copy link
Member

@Parasgr7
Can you fix the following yourself so that @Mohzcrea8me does not go around in circles ?

  1. Pair of test cases: _tokenInForExactTokenOut, calculations is not correct. Please go through the test cases once which I have done in branch pool_test.

@Mohzcrea8me
You can respond to the following -

  1. What is the reason for removing parsePoolPairData, limit amounts, _spotPriceAfterSwap test cases?
  2. Remove unnecessary comments, and please do proper code formatting.
  • Define variables for address[Currency/Security] to be used in test file
  • Optimise the primaryIssuePool code also

@Mohzcrea8me
Copy link
Collaborator Author

I went through your code @Mohzcrea8me , few things l would like you to change

  1. Pair of test cases: _tokenInForExactTokenOut, calculations is not correct. Please go through the test cases once which I have done in branch pool_test.
  2. What is the reason for removing parsePoolPairData, limit amounts, _spotPriceAfterSwap test cases?
  3. Remove unnecessary comments, and please do proper code formatting.
  • Define variables for address[Currency/Security] to be used in test file
  • Optimise the primaryIssuePool code also

Oh okay thanks, which of the _tokenInForExactTokenOut is incorrect? When currency is in or security is in?

  1. I removed _spotPriceAfterSwap and others so it will be easier for you to review you said I need to make the reviewing easy for you I just want you to see the changes I added

  2. @kallolborah told me to add comments to better explain what I was trying to do, I will remove them

I don't understand what define variable for address[currency/security] means

Okay I will format the code and remove all unnecessary comments, the problem I have is that our calculations are the same but when I test yours and mine with same balances and amounts I get different answers I just want to be sure if I'm right before moving on @Parasgr7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants