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 invoke send default behavior #1536

Merged
merged 9 commits into from
Aug 9, 2024
Merged

Conversation

leighmcculloch
Copy link
Member

@leighmcculloch leighmcculloch commented Aug 9, 2024

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 to default.

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 it default 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.

@leighmcculloch leighmcculloch changed the title Fix invoke send default behavior to send for auth Fix invoke send default behavior Aug 9, 2024
@leighmcculloch leighmcculloch marked this pull request as ready for review August 9, 2024 05:53
@leighmcculloch leighmcculloch enabled auto-merge (squash) August 9, 2024 06:07
@leighmcculloch
Copy link
Member Author

E2e tests should be fixed by:

@leighmcculloch leighmcculloch merged commit 2e63fbe into main Aug 9, 2024
26 checks passed
@leighmcculloch leighmcculloch deleted the unmannered-underswearer branch August 9, 2024 15:15
@janewang
Copy link
Contributor

janewang commented 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
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants