|
| 1 | +# PR Review Rules |
| 2 | + |
| 3 | +These are the architectural patterns and rules for the Privy Unity SDK. Any new code must adhere to these. |
| 4 | + |
| 5 | +## Architecture (Blocking) |
| 6 | + |
| 7 | +- **Interface + Implementation separation**: Every public service has an interface prefixed with `I` (e.g., `ILoginWithEmail`, `IPrivy`) and a separate `internal` implementation class (e.g., `LoginWithEmail`, `PrivyImpl`). Interfaces are `public`, implementations are `internal`. |
| 8 | +- **Folder structure**: Features live in `SDK/Runtime/<Feature>/`. Interfaces and implementations live in the same feature folder. Models get a `Models/` subfolder within the feature. |
| 9 | +- **Namespace per folder**: Each folder has its own namespace matching the path — `Privy.Auth.Email`, `Privy.Core`, `Privy.Wallets`, `Privy.Utils`, `Privy.Internal.Networking`, etc. |
| 10 | +- **Internal namespaces**: Implementation-only types use `Privy.Internal.*` namespaces (e.g., `Privy.Internal.Networking`, `Privy.Internal.Storage`). |
| 11 | + |
| 12 | +## Naming Conventions (Blocking) |
| 13 | + |
| 14 | +- **Interfaces**: `I` prefix + PascalCase — `IPrivy`, `ILoginWithEmail`, `IHttpRequestHandler`, `IAuthDelegator`. |
| 15 | +- **Implementation classes**: No prefix, PascalCase matching the concept — `PrivyImpl`, `LoginWithEmail`, `HttpRequestHandler`, `AuthDelegator`. |
| 16 | +- **Private fields**: `_camelCase` — `_authDelegator`, `_httpRequestHandler`. |
| 17 | +- **Public properties/methods**: PascalCase — `Email`, `GetUser()`, `AuthStateChanged`. |
| 18 | +- **Constants**: PascalCase — `MaxRetries`, `ApiVersion`. |
| 19 | +- **File names**: Match the type name — `ILoginWithEmail.cs`, `LoginWithEmail.cs`, `PrivyException.cs`. |
| 20 | + |
| 21 | +## Access Control (Blocking) |
| 22 | + |
| 23 | +- **Public interfaces only**: Only interfaces (`IPrivy`, `ILoginWithEmail`, etc.) and types needed by SDK consumers are `public`. |
| 24 | +- **Internal implementations**: All implementation classes are `internal`. They must not be directly accessible to SDK consumers. |
| 25 | +- **Public models/enums for consumers**: Data types returned to consumers (`AuthState`, `PrivyException`, `AuthenticationError` enum) are `public`. |
| 26 | +- **Internal networking/storage**: `IHttpRequestHandler`, `PlayerPrefsDataManager`, repositories are `internal`. |
| 27 | + |
| 28 | +## Dependency Injection (Blocking) |
| 29 | + |
| 30 | +- **Constructor injection**: All dependencies are passed via constructor. No service locators or static singletons (except the `PrivyManager` entry point). |
| 31 | +- **Wiring in PrivyImpl**: All service instantiation happens in `PrivyImpl`'s constructor. New services must be instantiated there with their dependencies. |
| 32 | +- **Null-check constructor params**: Constructor parameters for required dependencies should include `?? throw new ArgumentNullException(nameof(param))`. |
| 33 | +- **PrivyManager as entry point**: `PrivyManager.Initialize(config)` is the only way to create an SDK instance. No other public constructors. |
| 34 | + |
| 35 | +## Async Patterns (Blocking) |
| 36 | + |
| 37 | +- **Task-based async**: All async operations return `Task<T>`. Use `async/await` throughout. |
| 38 | +- **TaskCompletionSource for initialization**: SDK initialization uses `TaskCompletionSource` to allow `GetAuthState()`/`GetUser()` to await readiness. |
| 39 | +- **SafeFireAndForget for background work**: Fire-and-forget tasks use the `.SafeFireAndForget()` extension with error logging. |
| 40 | +- **No blocking calls**: Never use `.Result` or `.Wait()` on tasks. Always `await`. |
| 41 | + |
| 42 | +## Error Handling (Blocking) |
| 43 | + |
| 44 | +- **Typed exceptions**: Use `PrivyAuthenticationException` (with `AuthenticationError` enum) for auth failures and `PrivyWalletException` (with `EmbeddedWalletError` enum) for wallet failures. Base class is `PrivyException`. |
| 45 | +- **Error enums**: Add new error cases to the appropriate enum (`AuthenticationError` or `EmbeddedWalletError`) rather than using generic error messages. |
| 46 | +- **No swallowed exceptions**: Every catch block must either rethrow (wrapped), log, or handle meaningfully. Never empty catch blocks. |
| 47 | +- **Guard clauses**: Validate inputs at the top of methods with descriptive exceptions. |
| 48 | + |
| 49 | +## Layered Architecture (Warning) |
| 50 | + |
| 51 | +- **IPrivy → LoginWith* → AuthDelegator → AuthRepository → HttpRequestHandler**: Public API delegates to feature modules, which use the auth delegator, which calls repositories, which use the HTTP handler. Don't skip layers. |
| 52 | +- **AuthDelegator for state management**: Authentication state changes flow through `AuthDelegator`. Modules should not directly mutate auth state. |
| 53 | +- **Repositories for network calls**: Repositories (`AuthRepository`, `AppConfigRepository`) handle HTTP requests and deserialization. Business logic belongs in delegators/managers. |
| 54 | + |
| 55 | +## Network Layer (Warning) |
| 56 | + |
| 57 | +- **Use IHttpRequestHandler**: All API calls go through `IHttpRequestHandler.SendRequestAsync()`. Never use `UnityWebRequest` directly in feature code. |
| 58 | +- **JSON serialization**: Use `JsonUtility` or the project's JSON approach consistently. Request/response models should be serializable. |
| 59 | +- **Custom headers via parameter**: Pass additional headers (e.g., MFA tokens) through the `customHeaders` dictionary parameter, not by modifying the handler. |
| 60 | + |
| 61 | +## Events (Warning) |
| 62 | + |
| 63 | +- **C# events for state changes**: Use `event Action<T>` for state change notifications (e.g., `AuthStateChanged`). Forward events from internal components to the public interface. |
| 64 | +- **No Unity-specific patterns in SDK core**: Don't use `UnityEvent`, `MonoBehaviour`, or coroutines in the SDK Runtime. Use standard C# async/await and events. |
| 65 | + |
| 66 | +## Documentation (Nit) |
| 67 | + |
| 68 | +- **XML docs on public types**: All `public` interfaces, methods, properties, and classes need `/// <summary>` documentation. |
| 69 | +- **Document parameters**: Use `/// <param name="">` for method parameters. |
| 70 | +- **Document exceptions**: Use `/// <exception cref="">` for thrown exceptions. |
| 71 | +- **No over-documentation**: Internal implementation classes don't need XML docs unless behavior is non-obvious. |
| 72 | + |
| 73 | +## Style (Nit) |
| 74 | + |
| 75 | +- **Allman brace style**: Opening braces on a new line (enforced by `.editorconfig`). |
| 76 | +- **4-space indentation**: No tabs. |
| 77 | +- **Format with `dotnet format`**: Run `dotnet format Format.csproj` before committing. |
| 78 | +- **Expression-bodied members**: Prefer for simple single-expression properties/methods. |
| 79 | +- **Object/collection initializers**: Required (enforced as error in `.editorconfig`). |
| 80 | + |
| 81 | +## Sample App (Nit) |
| 82 | + |
| 83 | +- **Update SampleApp on public API changes**: When adding new public interfaces or changing method signatures, update `SampleApp/` to demonstrate usage. |
0 commit comments