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 bug in Transaction.isSorobanTransaction to accommodate BumpFootprintExpirationOperation. #518

Merged

Conversation

overcat
Copy link
Member

@overcat overcat commented Aug 30, 2023

Fix #517

We need to release a new version, and I have already included it in 96eebcd.

Due to the instability of Android testing on API 33 / GitHub Action, I have removed it from the deploy needs. I need some time to investigate this issue as it runs fine on my local machine.

account.getAccountId(),
Transaction.MIN_BASE_FEE,
account.getIncrementedSequenceNumber(),
new org.stellar.sdk.Operation[] {operation, operation},
Copy link
Contributor

Choose a reason for hiding this comment

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

i think only single op needed in tx correct? otherwise isSorobanTransaction=false, but for non-tested reason, i.e. it's more than one op, it won't validate op instance mismatch yet.

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 test is to verify that a transaction containing multiple operations is not a soroban transaction. You can take a look at https://github.com/stellar/java-stellar-sdk/pull/518/files#diff-0114ab460186855a151f487001dae40013372c8430be55eeebdb6e24bfc12705R448

assertFalse(transaction.isSorobanTransaction());

Copy link
Contributor

Choose a reason for hiding this comment

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

there is already another test for asserting multi-op - testIsSorobanTransactionMultiOperations outcome, I think this test was intending to exercise a non-multi op code path but an incorrect op type, which seemed to be the intent, just pass one operation and it should verify same false outcome.

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'm sorry, I made a mistake. I have already corrected it. 🤦

Copy link
Contributor

@sreuland sreuland left a comment

Choose a reason for hiding this comment

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

great test coverage to assert fix.

@overcat
Copy link
Member Author

overcat commented Sep 1, 2023

Hi @sreuland @tamirms, can you help me publish a new release?

@sreuland
Copy link
Contributor

sreuland commented Sep 1, 2023

Hi @sreuland @tamirms, can you help me publish a new release?

yes, it's ok for me to merge this PR to soroban correct?

@sreuland sreuland merged commit 1461c2f into lightsail-network:soroban Sep 1, 2023
6 checks passed
@sreuland
Copy link
Contributor

sreuland commented Sep 1, 2023

@overcat , 0.41.0-beta.3 was published, thank you!

@overcat overcat deleted the fix-is-soroban-tx 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.

2 participants