IAppAuthBasicManagement: complete the management surface (#1)#658
Closed
dmvt wants to merge 0 commit into
Closed
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Expands the on-chain management interface for app authentication contracts so third-party IAppAuth implementations can be drop-in compatible with the operator tooling and existing tests that currently assume the concrete DstackApp surface.
Changes:
- Extended
IAppAuthBasicManagementto include configuration setters, read getters, and identity/version reads, plus the related events. - Updated
DstackAppto explicitly satisfy the expanded interface (state var getter overrides, mutator overrides,owner()/version()overrides) and to compute ERC-165 IDs viatype(...).interfaceId. - Added a unit test that pins the new
IAppAuthBasicManagementinterface ID (true) and asserts the previous ID is no longer claimed (false).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| kms/auth-eth/test/DstackApp.test.ts | Adds supportsInterface assertions for current/old interface IDs and basic ERC-165 sanity checks. |
| kms/auth-eth/contracts/IAppAuthBasicManagement.sol | Broadens the management interface to match the surface dstack tooling/tests rely on (events, mutators, getters, owner/version). |
| kms/auth-eth/contracts/DstackApp.sol | Implements the expanded interface via overrides and switches supportsInterface to type(...).interfaceId to avoid selector drift. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
IAppAuthBasicManagement.soldeclared only the four allowlist mutators (addComposeHash/removeComposeHash/addDevice/removeDevice). The reference implementationDstackApp.solexposes a much wider externally-observable surface that operator tooling and the existing test suite already rely on, but those methods weren't in the interface — so any third-partyIAppAuthimplementer could conform in name without satisfying the de facto contract dstack tooling expects.This PR expands
IAppAuthBasicManagementto declare every method that's already exercised againstDstackAppfrom within the dstack repo. Going forward, third-party app contracts (such as the cluster-patternTeeSqlClusterMemberthat motivated this PR) can mirror this surface as passthroughs to a parent governance contract and remain drop-in compatible withphala-cli+ dstack tooling.Where the gap shows up inside dstack
kms/auth-eth/hardhat.config.ts:476(app:set-allow-any-deviceoperator task)setAllowAnyDevice(bool)test/DstackApp.test.ts:33,upgrade.test.ts:63,72owner()test/DstackApp.test.ts:37,upgrade.test.ts:80,208version()test/DstackApp.test.ts:46,52,249,339,upgrade.test.ts:66,75,211allowedComposeHashes(bytes32)test/DstackApp.test.ts:245,338,upgrade.test.ts:65,74allowedDeviceIds(bytes32)test/DstackApp.upgrade.test.ts:64,73,210allowAnyDevice()test/DstackApp.upgrade.test.ts:86,98,209requireTcbUpToDate()test/DstackApp.upgrade.test.ts:92,97,123,150setRequireTcbUpToDate(bool)(All four pre-existing
addComposeHash/removeComposeHash/addDevice/removeDevicewere already in the interface.)External tooling (third-party CLIs that target a CVM's
app_idcontract during in-place updates, e.g. the CLI workflow we describe in TeeSQL'sdstackgresbug-report doc) hits the same gap and has to assume the concreteDstackApptypeinstead of the abstract interface. With this change such tooling can rely on
supportsInterface(IAppAuthBasicManagement.interfaceId) → trueto know every method below is safe to call.What's added
DstackApp.sol changes
overrideagainst the extended interface.setAllowAnyDevice,setRequireTcbUpToDate) markedoverride.version()markedoverride(was justpublic pure; now also overrides the interface'sviewdeclaration —puresatisfiesview).owner()overridden against(OwnableUpgradeable, IAppAuthBasicManagement); body just delegates toOwnableUpgradeable.owner()so behaviour is unchanged.publicstate vars (allowedComposeHashes,allowedDeviceIds,allowAnyDevice,requireTcbUpToDate) markedoverrideso their auto-generated getters satisfy the interface.AllowAnyDeviceSet+RequireTcbUpToDateSetevent declarations dropped — inherited from the interface to avoid duplicate-event errors.supportsInterface()switched from hardcoded selectors (0x1e079198,0x8fd37527) totype(...).interfaceIdso the literal can't drift from the interface as it evolves.IAppAuthBasicManagementinterface ID changes from0x8fd37527→0xea8447a1.Any third-party contract that hardcoded the old literal in its own
supportsInterface()will silently stop advertising support. The recommended fix is to usetype(IAppAuthBasicManagement).interfaceId(as
DstackApp.solnow does) so the value stays in sync as the interface evolves.A new unit test pins both the new ID (must be
true) and the old ID (must befalse) so any future accidental ID drift fails loudly.Test plan
npx hardhat test test/DstackApp.test.ts test/DstackApp.upgrade.test.ts— 30/30 passing (including the newsupportsInterfaceassertion).npx jest— 41/41 passing (off-chain auth backend unaffected; still callsDstackKms.isAppAllowedwhich delegates viaIAppAuth).npx hardhat compile— clean compile of all 19 contract files; typechain regenerated.