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

test: RPC provider API key modifications #269

Merged
merged 14 commits into from
Sep 12, 2024
Merged

test: RPC provider API key modifications #269

merged 14 commits into from
Sep 12, 2024

Conversation

rvanasa
Copy link
Collaborator

@rvanasa rvanasa commented Sep 10, 2024

Follow-up testing and bugfixes for #252.

This PR...

  • Adds state machine tests for the following scenarios:
    • Set the key for a provider that uses RpcAuth::BearerToken.
    • Update a previously existing API key of some provider to None removes it.
    • Panic if trying to specify an API key for a provider that uses RpcAccess::Unauthenticated
    • Panic if a provider is not found.
  • Sets up the following canister upgrade tests:
    • Keep previously inserted API keys.
    • Change or keep demo status based on upgrade args.
    • Change or keep principals allowed to manage API key providers based on upgrade args.
  • Includes unit tests for BoolStorable and PrincipalStorable.
  • Removes unused StringStorable.
  • Changes the requestCost canister method to return 0 when in demo mode.
  • Fixes the following bugs:
    • BoolStorable had an inverted condition in from_bytes
    • getProviderCost() returned Nat instead of Result<Nat, RpcError> in the state machine test logic

Note that the canister upgrade tests use the same Wasm before and after upgrade. In the future (probably out of scope of this PR), we could extend this to test upgrading from the most recently deployed canister's Wasm file.

@rvanasa rvanasa marked this pull request as draft September 10, 2024 19:33
@rvanasa rvanasa marked this pull request as ready for review September 11, 2024 00:03
@@ -581,4 +586,43 @@ mod test {
let api_key = ApiKey("55555".to_string());
assert!(format!("{api_key:?}") == "{API_KEY}");
}

#[test]
fn test_bool_storable() {
Copy link
Member

Choose a reason for hiding this comment

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

Rather than adding those tests regarding implementation of Storable, wouldn't it be better to just update to the latest version of stable structure and remove those wrappers?

Copy link
Collaborator Author

@rvanasa rvanasa Sep 11, 2024

Choose a reason for hiding this comment

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

I timeboxed trying to use the latest version of ic-stable-structures, and the issue that I ran into was that all of the latest versions which implement Storable for Principal and bool use Candid 10.x, which conflicts with transitive dependencies from the forked cketh-minter branch.

After spending quite a lot of time trying to work around this (and running into more and more version conflicts), it might make sense to wait until after we can drop the cketh-minter dependency before making this change in another PR. However, I was able to remove StringStorable to simplify the codebase.

Copy link
Member

Choose a reason for hiding this comment

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

use Candid 10.x, which conflicts with transitive dependencies from the forked cketh-minter branch.

ha I see, another good reason to drop that dependency 😅.

it might make sense to wait until after we can drop the cketh-minter dependency before making this change in another PR

Sure that sounds ok to me. I'm trying to also push on that front, since it will be needed when we want to implement something new (e.g. 3-out-of-4 strategy)

tests/tests.rs Show resolved Hide resolved
tests/tests.rs Outdated Show resolved Hide resolved
@rvanasa
Copy link
Collaborator Author

rvanasa commented Sep 12, 2024

Merging and will make any follow-up changes in additional PRs.

@rvanasa rvanasa merged commit e2158f0 into main Sep 12, 2024
3 checks passed
@rvanasa rvanasa deleted the test-nns-providers branch September 12, 2024 18:09
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