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(jellyfish-api-core): add token split testing #1475

Merged
merged 134 commits into from
Jun 8, 2022

Conversation

surangap
Copy link
Contributor

@surangap surangap commented May 26, 2022

What this PR does / why we need it:

Which issue(s) does this PR fixes?:

Fixes #1527 and DeFiCh/ain#1243

Additional comments?:

@jingyi2811
Copy link
Contributor

Should we change v0/oracles/splits/ to v0/tokens/splits/ , I feel we should. @surangap Can you advise C++ side on this?

@surangap
Copy link
Contributor Author

surangap commented Jun 7, 2022

Anyway, don't spend too long time on this PR anymore as this is not urgent anymore because splitting already happened. This PR is important again only when we have the next split.

you are only seeing one use of the tests. but once these tests are written and included in the CI testing, these tests run over each and every future change of defid/jellyfish. Thus make sure the core functionality is intact over the changes. That's why it's important to get the tests right at the first place.

@surangap
Copy link
Contributor Author

surangap commented Jun 7, 2022

Also, you also need to read again.

If this is true => split happens at a single designated block splitBlock1 Then, something wrong with this PR

I need to change the code if you confirm that is true

https://github.com/DeFiCh/ain/blob/27e9965efd4b3b6d4beffefeb08ef902a6a37c49/src/validation.cpp#L4387
hope this helps.

@jingyi2811
Copy link
Contributor

#834

Apply the same too

Also, you also need to read again.
If this is true => split happens at a single designated block splitBlock1 Then, something wrong with this PR
I need to change the code if you confirm that is true

https://github.com/DeFiCh/ain/blob/27e9965efd4b3b6d4beffefeb08ef902a6a37c49/src/validation.cpp#L4387 hope this helps.

In feature_token_split.py, every call of v0/oracles/splits/, they generate 2 blocks. I am unsure is there a problem there

@jingyi2811
Copy link
Contributor

Anyway, don't spend too long time on this PR anymore as this is not urgent anymore because splitting already happened. This PR is important again only when we have the next split.

you are only seeing one use of the tests. but once these tests are written and included in the CI testing, these tests run over each and every future change of defid/jellyfish. Thus make sure the core functionality is intact over the changes. That's why it's important to get the tests right at the first place.

I assume there is no other more important task

@surangap surangap marked this pull request as ready for review June 8, 2022 02:42
@jingyi2811
Copy link
Contributor

#834

Apply the same too

Also, you also need to read again.
If this is true => split happens at a single designated block splitBlock1 Then, something wrong with this PR
I need to change the code if you confirm that is true

https://github.com/DeFiCh/ain/blob/27e9965efd4b3b6d4beffefeb08ef902a6a37c49/src/validation.cpp#L4387 hope this helps.

In feature_token_split.py, every call of v0/oracles/splits/, generate 2 blocks. I am unsure is there a problem there

I guess is 2 blocks right?

Copy link
Contributor

@fuxingloh fuxingloh left a comment

Choose a reason for hiding this comment

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

This is gonna be hard to index on whale-api due to non-deterministic events especially getting the current txid to map to PoolPair. I guess to make things simple we can just remove those values (txid, tokenid, poolpairid).

Otherwise, we could also; might have to - build a DEX indexer away from the main whale-api to protect it from regression 😿

/cc @canonbrother @eli-lim

Related to #1532 regression

@fuxingloh fuxingloh changed the title fix(jellyfish-api-core): Add token split testing feat(jellyfish-api-core): add token split testing Jun 8, 2022
@jellyfishsdk-bot jellyfishsdk-bot added kind/feature New feature request and removed kind/fix Fix a bug labels Jun 8, 2022
@fuxingloh fuxingloh merged commit 91c5566 into main Jun 8, 2022
@fuxingloh fuxingloh deleted the surangap/token-split-tests branch June 8, 2022 10:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lock token and Oracles split
7 participants