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 the issue of unable to parse liquidity_pool_revoked effect properly. #521

Merged

Conversation

overcat
Copy link
Member

@overcat overcat commented Sep 5, 2023

fix #393

Due to user demand for this fix, let's release it on the master branch.

Let's merge #522 first.

We have already added it in the soroban branch, but since we need to release it in the master branch now, let's add it again.
Copy link
Member Author

Choose a reason for hiding this comment

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

This file is irrelevant to this PR, please ignore it.

Copy link
Member Author

Choose a reason for hiding this comment

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

This file is irrelevant to this PR, please ignore it.

Copy link
Contributor

Choose a reason for hiding this comment

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

what issue or pr is it related to, this will land on master, is there a way to rebase the source branch to get cleaner change set?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is because when we merged #426, #426 was not formatted (it is a very "old" PR.). If necessary, I can create another PR to format it.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, I see it mentioned in the changelog notes, which is great, thanks!

@sreuland
Copy link
Contributor

sreuland commented Sep 6, 2023

It looks like this pr is also doing the release prep, as it has notes and version bump for 0.40.1, just to confirm, you intend to merge this and #522 to master and then we just tag release off master which would be an effective changeset of just those two, as nothing else has merged to master since 0.40.0 correct?

@overcat
Copy link
Member Author

overcat commented Sep 6, 2023

It looks like this pr is also doing the release prep, as it has notes and version bump for 0.40.1, just to confirm, you intend to merge this and #522 to master and then we just tag release off master which would be an effective changeset of just those two, as nothing else has merged to master since 0.40.0 correct?

Yes, let's merge these two PRs and then release it on the master branch. I have updated the changelog to include all the changes.

@@ -1318,4 +1321,96 @@ public void testDeserializeLiquidityPoolTradeEffect() {
new AssetAmount(
create("ARST:GB7TAYRUZGE6TVT7NHP5SMIZRNQA6PLM423EYISAOAP3MKYIQMVYP2JO"), "1.0000000"));
}

Copy link
Contributor

Choose a reason for hiding this comment

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

great test coverage!

@sreuland sreuland merged commit 4d87082 into lightsail-network:master Sep 7, 2023
sreuland pushed a commit that referenced this pull request Sep 12, 2023
* fix the issue of unable to parse liquidity_pool_revoked effect properly. (#521)

* add jitpack config.

* bump project version to upcoming 0.4.1

* update change log, include #522

* define `cursor`, `order` and `limit` in AssetsRequestBuilder object. (#522)
@overcat overcat deleted the fix/issue/393 branch September 13, 2023 06:52
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.

When the query effect data type returns 95, there will be a gson parsing exception
2 participants