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

feat(backend): Gate api methods #1807

Merged
merged 24 commits into from
Aug 6, 2024
Merged

feat(backend): Gate api methods #1807

merged 24 commits into from
Aug 6, 2024

Conversation

bitdivine
Copy link
Member

@bitdivine bitdivine commented Jul 18, 2024

Motivation

When we split the backend into two canisters, we will need to disable APIs gradually and selectively:

timeline
    title Data Migration
    Now : backend (user data read-write, eth read-sign)
    Pre-migration : backend (user data read-write, eth read-sign)
       : new_backend (all user-facing APIs disabled)
    In-migration : backend (readonly)
       : new_backend (all user-facing APIs disabled)
    Data transfer completed : backend (readonly)
       : new_backend (user data read-write)
    UI uses new backend
       : backend (signing read-sign)
       : new_backend (user data read-write)
Loading

Note:

  • When threshold keys are readonly, users will be able to get their Ethereum public key and so see their balances but will not be able to transact.
  • When user data is readonly, users will be able to see their tokens as usual but not be able to create new accounts or edit their accounts.

Changes

  • Add init args to selectively disable API methods.
  • Apply guards to all user facing methods that access either user data or threshold signatures.

Tests

  • Unit tests are included to ensure that the enums behave as expected.

TODO

  • PR with an API method that enables or disables features without reinstall.
  • Test that after signing is disabled, the new canister will indeed not provide public keys or sign anything.

@bitdivine bitdivine changed the title feat (backend): Gate api methods feat(backend): Gate api methods Jul 18, 2024
@anedos-dfinity
Copy link
Contributor

If I understand correctly, this architecture allows for the Oisy BE migration to occur while Oisy is still live - but read-only - is this the idea behind it ?

@bitdivine
Copy link
Member Author

This does support readonly during migration but the API toggles are needed with or without. For example, if reads are still enabled on the signer or signing on the backend after migration and we call the wrong canister because a user hasn't refreshed so is using an old client, we are in a worse state than before.

@bitdivine bitdivine marked this pull request as ready for review August 5, 2024 12:23
@bitdivine bitdivine requested a review from a team as a code owner August 5, 2024 12:23
@@ -10,6 +10,7 @@ type AddUserCredentialRequest = record {
current_user_version : opt nat64;
credential_spec : CredentialSpec;
};
Copy link
Member Author

Choose a reason for hiding this comment

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

We wish to be able to make features either fully enabled, fully disabled, or enabled for read methods only. This type is used to represent which of these states a feature is in.

@@ -108,6 +108,7 @@ pub(crate) fn init_arg() -> Arg {
issuer_origin: ISSUER_ORIGIN.to_string(),
credential_type: CredentialType::ProofOfUniqueness,
}]),
api: None,

Choose a reason for hiding this comment

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

@bitdivine: Will you add some integration tests that exercise the API disabling?

Copy link
Member Author

Choose a reason for hiding this comment

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

Disabling APIs matters in the migration, so I was going to test that in the migration e2e tests. After the migration is complete I expect all of this will be deleted again. 😄

Copy link

@frederikrothenberger frederikrothenberger left a comment

Choose a reason for hiding this comment

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

LGTM, a little bit sad that the CDK annotations do not allow multiple guards...

@bitdivine
Copy link
Member Author

Agreed! At least, I didn't test but it looks that way based on this option: https://github.com/dfinity/cdk-rs/blob/59795716487fbb8a9910ac503bcea1e0cb08c932/src/ic-cdk-macros/src/export.rs#L12 No idea whether changing the option to a vec would "just work" or whether more is needded. I see no hints either way in the macro attribute code.

@bitdivine bitdivine merged commit 722331a into main Aug 6, 2024
13 checks passed
@bitdivine bitdivine deleted the gate-api branch August 6, 2024 09:34
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