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

Phase out Halt abstractions #1953

Merged
merged 21 commits into from
Oct 24, 2023
Merged

Phase out Halt abstractions #1953

merged 21 commits into from
Oct 24, 2023

Conversation

sug0
Copy link
Contributor

@sug0 sug0 commented Sep 28, 2023

Describe your changes

Halt was an abstraction conceived when the code we currently deem the "SDK" was initially ported from apps to shared. When the SDK code lived in apps, many error scenarios resulted in the client aborting with safe_exit(1), so a close approximation of this behavior was a halt instruction of sorts, which, as the name implies, would halt all execution.

However, since then, the SDK has improved in terms of usability. Panics and other odd bits were replaced with a proper Error type, returned from a Result monad. So, this PR attempts to remove the Halt legacy code, and integrate the code that depended on it with the new SDK Error type.

Indicate on which release or other PRs this topic is based on

Based on #1963

Checklist before merging to draft

  • I have added a changelog
  • Git history is in acceptable state

@sug0 sug0 added the enhancement New feature or request label Sep 28, 2023
@sug0 sug0 changed the base branch from main to tiago/move-eth-to-sdk September 28, 2023 09:22
sug0 added a commit that referenced this pull request Sep 28, 2023
@sug0 sug0 marked this pull request as ready for review September 28, 2023 09:24
@sug0 sug0 changed the base branch from tiago/move-eth-to-sdk to main September 28, 2023 09:25
sug0 added a commit that referenced this pull request Sep 28, 2023
sug0 added a commit that referenced this pull request Sep 28, 2023
@sug0 sug0 changed the base branch from main to tiago/move-eth-to-sdk September 28, 2023 09:27
@sug0 sug0 added the SDK label Sep 28, 2023
shared/src/sdk/rpc.rs Outdated Show resolved Hide resolved
tzemanovic
tzemanovic previously approved these changes Sep 29, 2023
sug0 added a commit that referenced this pull request Sep 29, 2023
sug0 added a commit that referenced this pull request Sep 29, 2023
sug0 added a commit that referenced this pull request Sep 29, 2023
Copy link
Member

@batconjurer batconjurer left a comment

Choose a reason for hiding this comment

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

One question, one suggestion. Otherwise, looks good

sug0 added a commit that referenced this pull request Oct 9, 2023
sug0 added a commit that referenced this pull request Oct 12, 2023
@sug0 sug0 marked this pull request as draft October 13, 2023 08:40
@sug0 sug0 changed the base branch from tiago/move-eth-to-sdk to murisi/sdk-refactor-rebased October 13, 2023 13:06
@sug0 sug0 marked this pull request as ready for review October 13, 2023 13:07
@sug0 sug0 requested a review from murisi October 13, 2023 13:08
sug0 added a commit that referenced this pull request Oct 13, 2023
@sug0 sug0 mentioned this pull request Oct 13, 2023
@Fraccaman Fraccaman changed the base branch from murisi/sdk-refactor-rebased to main October 23, 2023 11:48
@Fraccaman Fraccaman dismissed tzemanovic’s stale review October 23, 2023 11:48

The base branch was changed.

Fraccaman added a commit that referenced this pull request Oct 23, 2023
* origin/tiago/phase-out-try-halt:
  Changelog for #1953
  Echo SDK errors to stdout
  Internal macros SDK module
  Apply suggestions from code review
  Remove halt abstraction from the SDK
  Phase out Halt and its cousins from the SDK
  Add new SDK errors
@tzemanovic tzemanovic mentioned this pull request Oct 24, 2023
@tzemanovic tzemanovic merged commit af1fa0e into main Oct 24, 2023
4 checks passed
@tzemanovic tzemanovic deleted the tiago/phase-out-try-halt branch October 24, 2023 11:26
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.

None yet

4 participants