-
Notifications
You must be signed in to change notification settings - Fork 77
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 invoke send default behavior #1536
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
leighmcculloch
changed the title
Fix invoke send default behavior to send for auth
Fix invoke send default behavior
Aug 9, 2024
leighmcculloch
commented
Aug 9, 2024
leighmcculloch
commented
Aug 9, 2024
leighmcculloch
commented
Aug 9, 2024
E2e tests should be fixed by: |
fnando
approved these changes
Aug 9, 2024
Thank you for fixing @leighmcculloch |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
Change the recently merged (and not yet released) invoke
--send
default behavior so that by default send occurs if there are any contract events, send occurs if there is any soroban auth involved, and change the name of the default enum value todefault
.Also add output that tells the user simulation-only has occurred automatically.
Why
There's a bug in the code that checks if there are any events, a not that shouldn't be present, so the default behavior is sending when there are no events, rather when there are events.
Also, if auth is required it's more ambiguous to if a user would expect or not a transaction to be sent. Since read-only operations are rarely gated by auth, we can probably assume auth means the transaction should be sent, even if we can't otherwise detect a write to storage or a published event. It could be that the auth itself will do one of these two things and sending to the network could be desirable because of that.
The default behavior of sending should for the most part be as least surprising as possible. It's meant to be an optimisation for when folks are truly calling those read-only operations where sending makes little sense, but if there's any doubt it should just send.
I renamed the enum value because this isn't released yet and because
if-write
doesn't feel like it fully captures the default behavior given the addition of the auth logic. Given the default behavior is somewhat complex I named itdefault
influenced by some of the git cli's options where it uses that term for default behavior that isn't easy to capture in another word.