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

Auth Next changes in SDK #848

Merged
merged 23 commits into from
Feb 4, 2023
Merged

Conversation

dmkozh
Copy link
Contributor

@dmkozh dmkozh commented Feb 2, 2023

What

Adapt Soroban SDK to the host changes introduced in stellar/rs-soroban-env#645

Overview of the changes:

  • Replace the AccountId, invoker, and soroban-auth library with the single Address type that supports host-based authentication and authorization.
  • Use the recording auth for testing and provide a simple utility to verify that authorization has happened. Signatures are no longer required for tests thanks to that.
  • Replace network passphrase with id (this just reflects the host-side change; we don't really need to have both available to contrats)
  • Introduced a stub for the account contract SDK

Why

Auth Next motivation is described in the env PR. From the SDK perspective the auth and testing are greatly simplified.

Known limitations

Some of the possible followups:

  • The account SDK would need to be developed more to provide a better support for building the custom account contracts
  • Strkey should be used for displaying the addresses
  • Add testing utility to verify non-top-level authorizations
  • Add some macros to generate the auth code for simple cases (even though this is just a line of code, it would be more readable to have an explicit function annotation that requires a given address to authorize a call with all the input arguments).
  • Maybe bring back some of the classic account related functionality for the integration testing. While this should be unnecessary for the unit tests, there may be some need for it in snapshot-based testing. Not quite certain about that yet though.

@leighmcculloch leighmcculloch self-requested a review February 2, 2023 21:22
@dmkozh dmkozh force-pushed the auth_next_impl branch 3 times, most recently from 6956c1d to 4c3331a Compare February 3, 2023 01:10
The Address is meant to be used in place of the invoker/AccountId/Identifier etc. It also provides a way to call `require_auth` to perform authentication/authorization.

This also has some other Env changes supporting the Host changes for Auth Next.
For now this just contains the context type definition that host passes to the account. This can be extended as needed, hence I'm using a separate library.
@dmkozh dmkozh marked this pull request as ready for review February 3, 2023 01:13
Copy link
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

This is looking really nice. I have a few asks inline.

soroban-account/Cargo.toml Outdated Show resolved Hide resolved
soroban-ledger-snapshot/src/lib.rs Show resolved Hide resolved
soroban-sdk/src/address.rs Outdated Show resolved Hide resolved
soroban-sdk/src/address.rs Outdated Show resolved Hide resolved
soroban-sdk/src/address.rs Outdated Show resolved Hide resolved
soroban-sdk/src/accounts.rs Show resolved Hide resolved
soroban-sdk/src/env.rs Show resolved Hide resolved
soroban-sdk/src/env.rs Outdated Show resolved Hide resolved
soroban-sdk/src/env.rs Show resolved Hide resolved
soroban-sdk/src/env.rs Show resolved Hide resolved
Copy link
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

I think my suggestions were misunderstood in a couple places.

I think the main things left are:

  1. Figuring out if we can keep network passphrase, or if we need to move to network id in the SDK. I agree network id should be used within the env.

  2. Can we replace the verify_top_auth function.

It would be good to figure out (1) before merging so that we don't flip-flop the network passphrase code. But it's also fine to flip flop it if we can't figure it out.

I think we can fix (2) post-merge, but I think it is important we fix that before release, so maybe worth doing in this PR too.

soroban-sdk/src/address.rs Outdated Show resolved Hide resolved
soroban-sdk/src/testutils.rs Outdated Show resolved Hide resolved
soroban-sdk/src/address.rs Outdated Show resolved Hide resolved
… a token contract with a given admin.

This makes the test setup simpler and also removes the redundant coverage of the built-in contract.
@dmkozh
Copy link
Contributor Author

dmkozh commented Feb 4, 2023

I think we can fix (2) post-merge, but I think it is important we fix that before release, so maybe worth doing in this PR too.

I would suggest merging this as is (given everything else looks ok) in order to unblock your changes or any other unrelated changes. I'll prioritize this to make sure it makes it into release.

Copy link
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

Looks good, +1 to following up with the remaining things.

Two things with cfgs stand out to me which I commented on inline, just because they look inconsistent with other code following the same pattern. I'll try changing them and if they work changed I'll merge. Otherwise I'll merge them as it is.

soroban-sdk/src/address.rs Outdated Show resolved Hide resolved
soroban-sdk/src/testutils.rs Outdated Show resolved Hide resolved
@leighmcculloch leighmcculloch enabled auto-merge (squash) February 4, 2023 04:00
@leighmcculloch leighmcculloch merged commit d801de5 into stellar:main Feb 4, 2023
@leighmcculloch
Copy link
Member

@dmkozh I pushed a commit before merging. See 1ae8ac0.

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.

3 participants