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

refactor: Full re-write of SDK #22

Merged
merged 34 commits into from
Jul 13, 2024
Merged

Conversation

Zambrella
Copy link
Collaborator

- What I did
I re-wrote the code from scratch to allow for better extensibility and maintainability (and for fun). There is no additional functionality (yet) from the previous version. The main changes include:

  • Breaking the repo down into workspaces
  • Created traits for crypto and TLS to allow "drop-in" replacements if needed in the future
  • Used RustCrypto crates for crypto because it is written in pure Rust so should be compatible with more hardware
  • Used rustls crate for TLS which should have broader compatibility that the previous crate that was being used
  • More tests

- How to verify it

  • Check through the code
  • Run the tests (info about how to do this in the README.md)
  • Run the examples (info about how to do this in the README.md) and check the data is read and written correctly and can be accessed by other SDKs

- Description for the changelog
Full re-write of SDK

Doug Todd added 30 commits February 4, 2024 21:54
@Zambrella Zambrella requested review from gkc and cpswan March 23, 2024 11:45
@cpswan
Copy link
Member

cpswan commented Mar 23, 2024

Thanks @Zambrella

Can you please take a look at the clippy warnings and fix them up (you might even be able to use it to implement the suggested fixes).

@cpswan cpswan changed the title Full re-write of SDK refactor: Full re-write of SDK Mar 23, 2024
@Zambrella
Copy link
Collaborator Author

@cpswan Clippy warnings have been fixed :)

Copy link
Member

@cpswan cpswan left a comment

Choose a reason for hiding this comment

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

@Zambrella thanks for this. It's a brilliant contribution :)

Secondly my apologies that it's taken me so long to review. It's a big PR and I wanted the chance to properly test it. It took some travel disruption and an unexpected change to my weekend to clear a bit of time.

There are a few minor tweaks I'd love to have before merging, but if you don't have the bandwidth to come back to this right now then that's entirely understandable. So I'm approving and will merge anyway in a week or so if I don't hear from you.

"at_sign",
"at_tls",
"at_verbs",
] }
[package]
name = "at_rust"
version = "0.2.1"
Copy link
Member

Choose a reason for hiding this comment

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

Please bump the version to 0.3.0

Copy link
Member

Choose a reason for hiding this comment

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

We really shouldn't have binaries in the repo. Could these be substituted with text encoded keys?

@cpswan cpswan merged commit a02dc53 into atsign-foundation:trunk Jul 13, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants