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.

Sign up for GitHub

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

Jump to bottom

Graydon code review #1044

Merged
merged 11 commits into from
Sep 8, 2023
Merged

Graydon code review #1044

merged 11 commits into from
Sep 8, 2023

Conversation

graydon
Copy link
Contributor

@graydon graydon commented Sep 7, 2023 •
edited
Loading

Steps in code review so far:

  • Rename some variables
  • Move code around for ease of review, add review markers
  • Equip Env::Error with : Into<crate::Error> so that we can pass HostError::error through from Env conversions
  • Do so in bytes.rs
  • Equip Env::Error with : From<crate::Error> so that we can project Error into it in certain cases
  • Do so in preference to an unconditional unreachable!() in convert, which can panic
  • Fix some code in convert that does two compares back to back when walking a tuple instead of scrutinizing one compare against two cases
  • Change a bunch more uses of ConversionError to Error (some need minor SDK changes, see Adapt to removal of ConversionError from number type conversions. rs-soroban-sdk#1083)

@graydon graydon requested review from sisuresh and a team as code owners September 7, 2023 04:41
@graydon graydon removed the request for review from sisuresh September 7, 2023 04:41
@graydon graydon force-pushed the graydon-code-review branch from 6a71c37 to 4d0ec44 Compare September 8, 2023 06:20
This was referenced Sep 8, 2023
Closed
Merged
@graydon graydon force-pushed the graydon-code-review branch from 4d0ec44 to 78fd4d1 Compare September 8, 2023 06:27
leighmcculloch
leighmcculloch approved these changes Sep 8, 2023
View reviewed changes
soroban-env-common/src/bytes.rs Show resolved Hide resolved
soroban-env-common/src/error.rs Show resolved Hide resolved
@leighmcculloch leighmcculloch added this pull request to the merge queue Sep 8, 2023
Merged via the queue into main with commit 2fa4b79 Sep 8, 2023
@leighmcculloch leighmcculloch deleted the graydon-code-review branch September 8, 2023 19:52
github-merge-queue bot pushed a commit to stellar/rs-soroban-sdk that referenced this pull request Sep 9, 2023
@graydon
Adapt to removal of ConversionError from number type conversions. (#1083
1def3c6 
)

This contains small fixes to adapt to [changes to the env found during
code review](stellar/rs-soroban-env#1044) to
eliminate easy-to-adapt-to uses of ConversionError (as described in bug
stellar/rs-soroban-env#1046) where we can do
better and pass a full Error.
@graydon graydon restored the graydon-code-review branch September 28, 2023 07:21
@graydon graydon deleted the graydon-code-review branch September 28, 2023 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leighmcculloch leighmcculloch leighmcculloch approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@graydon @leighmcculloch