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

Generic IO #1746

Merged
merged 12 commits into from
Sep 25, 2023
Merged

Generic IO #1746

merged 12 commits into from
Sep 25, 2023

Conversation

batconjurer
Copy link
Member

This code replaces IO in sdk and client code with a trait that allows custom IO handling.

@batconjurer batconjurer marked this pull request as draft July 21, 2023 22:27
@batconjurer batconjurer marked this pull request as ready for review July 24, 2023 07:58
shared/src/types/io.rs Outdated Show resolved Hide resolved
tzemanovic
tzemanovic previously approved these changes Jul 24, 2023
tzemanovic
tzemanovic previously approved these changes Jul 25, 2023
mateuszjasiuk
mateuszjasiuk previously approved these changes Jul 26, 2023
Copy link
Contributor

@mateuszjasiuk mateuszjasiuk left a comment

Choose a reason for hiding this comment

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

Sorry for late review!
So I was able to implement the trait in browser.
In general all of this works great, the two things to comes to my mind though:

  • Sometimes it is a bit annoying to pass generic IO as last type namada::ledger::tx::build_transfer::<_, _, _, IO>, as first three types can be inferred
  • it would be nice to make prompt and read async. The reason is that when we use those functions in the extension background(service worker) we do not have access to prompt(or any other DOM related features). So to make it work we would have to do something like this: stackblitz. On the other hand though, at the moment we are not using prompts at all, so this might not be such a big issue.

@batconjurer
Copy link
Member Author

batconjurer commented Jul 26, 2023

So I can change the order of the generic arguments, but it's a choice of namada::ledger::tx::build_transfer::<_, _, _, IO> vs namada::ledger::tx::build_transfer::<IO, _, _, _>. I don't know if it's worth it for the same amount of typing in the end.

@batconjurer
Copy link
Member Author

However, I made the read and prompt methods async. That is easy.

@mateuszjasiuk
Copy link
Contributor

So I can change the order of the generic arguments, but it's a choice of namada::ledger::tx::build_transfer::<_, _, _, IO> vs namada::ledger::tx::build_transfer::<IO, _, _, _>. I don't know if it's worth it for the same amount of typing in the end.

Ah my bad, I thought they can be skipped the same as in TS 😅
Nvm then

mateuszjasiuk
mateuszjasiuk previously approved these changes Jul 26, 2023
shared/src/types/io.rs Outdated Show resolved Hide resolved
shared/src/types/io.rs Outdated Show resolved Hide resolved
tzemanovic
tzemanovic previously approved these changes Jul 27, 2023
mateuszjasiuk
mateuszjasiuk previously approved these changes Jul 28, 2023
Copy link
Contributor

@sug0 sug0 left a comment

Choose a reason for hiding this comment

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

Hey, I noticed apps/src/bin/namada-client/cli.rs hasn't been deleted yet, despite the module not being referenced from any rust source file.

sug0
sug0 previously approved these changes Aug 16, 2023
tzemanovic
tzemanovic previously approved these changes Aug 16, 2023
@batconjurer batconjurer dismissed stale reviews from tzemanovic and sug0 via 078cea0 September 1, 2023 07:17
@batconjurer batconjurer mentioned this pull request Sep 13, 2023
batconjurer added a commit that referenced this pull request Sep 18, 2023
Fraccaman added a commit that referenced this pull request Sep 25, 2023
* origin/bat/generic-io:
  [chore]: Merge in 0.21.1
  [fix]: Removed unused file
  [feat]: Changed default io read methods to use tokio's async io tools
  [feat]: Changed default io read methods to use tokio's async io tools
  [feat]: Made io reading methods async
  changelog: add #1746
  [chore]: Added IO macros to wallet cli. Renamed  to
  [fix]: Fixed intra-doc link typo
  [feat]: Made sdk and client  io generic
Fraccaman added a commit that referenced this pull request Sep 25, 2023
@brentstone brentstone merged commit a6a7c4c into main Sep 25, 2023
10 of 12 checks passed
@brentstone brentstone deleted the bat/generic-io branch September 25, 2023 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants