feat!: NNS-managed RPC providers #252

rvanasa · 2024-08-13T20:22:31Z

This PR makes significant changes to the EVM RPC canister to enable using NNS proposals in place of a trusted principal to add, remove, and update RPC providers.

Progress:

…ic-eth-rpc into nns-providers

This PR updates the GitHub CI on the `evm-rpc-canister` branch to be equivalent to the original GitLab configuration. This is a temporary measure to unblock development, since we intend to migrate this branch into the [EVM RPC repository](internet-computer-protocol/evm-rpc-canister#252) (which contains the vast majority of the testing for the forked codebase).

rvanasa · 2024-08-30T04:30:38Z

This PR should be ready to merge!

gregorydemay

Thanks a lot @rvanasa for this new version! I reviewed the whole MR in details and it looks very good! I left a few comments but most of them are minor, I don't see any blocker. Feel free to tell me if those comments are better addressed in a follow-up PR (in case you think there are meaningful to implement)

Cargo.toml

src/memory.rs

gregorydemay · 2024-09-02T06:06:07Z

tests/tests.rs

+}
+
+#[test]
+fn should_insert_api_keys() {


additional test ideas:

Set the key for a provider that uses Bearer Token.

Updating 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.

Sounds good. I'll do this in a separate PR so we can get this one merged.

gregorydemay · 2024-09-02T06:14:55Z

src/memory.rs

    pub static UNSTABLE_METRICS: RefCell<Metrics> = RefCell::new(Metrics::default());
-    static UNSTABLE_SUBNET_SIZE: RefCell<u32> = RefCell::new(NODES_IN_FIDUCIARY_SUBNET);
+    static UNSTABLE_DEMO_STATUS: RefCell<bool> = RefCell::new(false);


understanding question: why should this be unstable? For local development purposes, it would be I think a bit weird if one starts in demo mode, one does some local changes and upgrade, and the canister is back to non-demo mode. I would have a priori saved this flag in stable memory. WDYT?

I don't have a strong preference either way, but we might as well keep this in stable memory since that seems more intuitive for how people might use the canister in local development.

It also turns out Storable isn't implemented for bool, at least in the version of ic-stable-structures used in the project. 🙃 I added a BoolStorable for now and will include tests in the PR mentioned in this comment.

src/memory.rs

gregorydemay · 2024-09-02T06:43:28Z

src/providers.rs

-            cycles_per_message_byte: 0,
+pub const PROVIDERS: &[Provider] = &[
+    Provider {
+        provider_id: 0,


I think it would be maybe be a good thing to have a test ensuring that the provider_id are stable (maybe in another PR). WDYT?

How would you define "stable" in this context? As a starting point, I added a test to enforce the expected provider_id sequence (starting at zero, increasing by one). This covers the cases of removing or skipping a provider, while other changes are unlikely to occur by accident because each ID is hard-coded.

src/providers.rs

gregorydemay · 2024-09-02T06:54:43Z

src/providers.rs

-                .find(|(_, p)| p.primary && p.chain_id == id)
-                .or_else(|| providers.iter().find(|(_, p)| p.chain_id == id))
+        RpcService::Chain(id) => ResolvedRpcService::Provider(
+            find_provider(|p| p.chain_id == id)


understanding question: Before this was returning the first provider for which p.primary && p.chain_id == id, while now this return the first provider for which p.chain_id == id. Is-it clear that those are actually the same providers after this PR (do we need maybe a unit test for this?) or is-it somehow not that relevant here?

The logic is equivalent because we never ended up changing the primary flag on any provider in the production canister.

We could use this opportunity to remove the RpcService::Chain() variant given the ambiguity. Because this update will make a few other breaking changes, we could include this in the migration steps for canisters to prepare for the new release.

gregorydemay · 2024-09-02T06:58:56Z

src/types.rs

    fn from_bytes(bytes: Cow<[u8]>) -> Self {
-        Decode!(&bytes, Self).unwrap()
+        Self(String::from_bytes(bytes))


nit: we should call here the try_from from L.225 to ensure that the API key is valid (and panic otherwise).

A concern with doing this is that if we change the validation logic to exclude already registered API keys, this logic could potentially break the canister on upgrade and require a fresh reinstall in another NNS proposal. We could probably accept that risk, but it seems like a tradeoff. WDYT?

src/types.rs

Co-authored-by: gregorydemay <112856886+gregorydemay@users.noreply.github.com>

…protocol/ic-eth-rpc into nns-providers

rvanasa · 2024-09-05T17:02:15Z

Thank you for the detailed review @gregorydemay! I'll merge this and then will make follow-up changes in separate PRs as mentioned.

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 added 12 commits August 13, 2024 10:41

Update Provider data structures

7b7961b

Merge branch 'main' of https://github.com/internet-computer-protocol/…

9d5e14b

…ic-eth-rpc into nns-providers

Progress

3c9b372

Refactor / simplify

152712e

Progress

cec84b2

Progress

13f2ae4

Adjust provider URL / API key definitions

60875aa

Update 'main.rs'

760ee78

Update tests

bbd1c66

Check hostname in 'validate_url_pattern

035113d

Test new validation logic

e96300e

Update API key validation logic

6da4160

rvanasa changed the title ~~feat!: immutable providers via NNS proposals~~ feat!: NNS-managed RPC providers Aug 19, 2024

rvanasa added 10 commits August 19, 2024 11:30

Update Candid interface for clarity

91e6da3

Update Cargo.lock

7cc7fdd

Add 'ProviderId' struct

4b0dda9

Update tests

69df269

Implement NNS upgrade actions to add/update/remove providers

ff8ceeb

Add API key map in stable memory and 'insertApiKeys()' Candid method

d58ca13

Remove 'Auth::RegisterProvider' capability

32ed787

Remove 'authorize', 'deauthorize', and 'getAuthorized' Candid methods

4fbb880

Refactor auth system to use init arg

7e40d87

Add code comment

fbebbaf

This was referenced Aug 20, 2024

refactor(evm-rpc): update validation errors dfinity/ic#1027

Merged

chore(evm-rpc): set up GitHub CI dfinity/ic#1028

Merged

rvanasa added 3 commits August 20, 2024 14:01

Remove 'Auth::FreeRpc'

a789716

Misc

f32d2d1

Add 'SetServiceProvider' action

112fdf1

Use hard-coded RPC providers and service mappings

13e5200

rvanasa added 4 commits August 29, 2024 17:43

Try renaming Rust E2E canister in Cargo.toml

0ce1c77

Update dfx.json

1f6ba0a

Simplify Rust canister

56ea848

Adjust E2E script

46f4730

rvanasa marked this pull request as ready for review August 29, 2024 23:57

rvanasa added 2 commits August 29, 2024 18:20

Reformat

2f060b7

Remove extraneous test

051785e

gregorydemay reviewed Sep 2, 2024

View reviewed changes

gregorydemay approved these changes Sep 2, 2024

View reviewed changes

rvanasa and others added 14 commits September 4, 2024 11:57

Increase 'API_KEY_MAX_SIZE' from 256 to 512

7aec116

Log API key updates

08d0002

Use 'Authorization' header for Cloudflare provider

83fa123

Test 'provider_id' being equivalent to index in provider array

2135b6a

Refactor memory IDs

fc3c5ae

Refactor stable memory to use 'DefaultMemoryImpl'

ff38c02

Update src/main.rs

aeabd74

Co-authored-by: gregorydemay <112856886+gregorydemay@users.noreply.github.com>

Add Rustdoc for 'updateApiKeys'

a425c0b

Merge branch 'nns-providers' of https://github.com/internet-computer-…

e3c54ea

…protocol/ic-eth-rpc into nns-providers

Refactor 'demo_status' -> 'demo_active'

eaed081

Use stable memory for demo status

5dfc31e

Remove unnecessary mutable borrow

fedebf1

Remove 'wasm-bindgen' patch

14440e4

Reformat

b90e516

rvanasa merged commit 1e5ba9a into main Sep 5, 2024
3 checks passed

rvanasa deleted the nns-providers branch September 5, 2024 17:02

rvanasa mentioned this pull request Sep 10, 2024

test: RPC provider API key modifications #269

Merged

rvanasa mentioned this pull request Sep 19, 2024

Immutable RPC providers #244

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: NNS-managed RPC providers #252

feat!: NNS-managed RPC providers #252

rvanasa commented Aug 13, 2024 •

edited

Loading

rvanasa commented Aug 30, 2024

gregorydemay left a comment

gregorydemay Sep 2, 2024

rvanasa Sep 5, 2024

gregorydemay Sep 2, 2024

rvanasa Sep 4, 2024 •

edited

Loading

gregorydemay Sep 2, 2024

rvanasa Sep 4, 2024

gregorydemay Sep 2, 2024

rvanasa Sep 4, 2024

gregorydemay Sep 2, 2024

rvanasa Sep 4, 2024 •

edited

Loading

rvanasa commented Sep 5, 2024

feat!: NNS-managed RPC providers #252

feat!: NNS-managed RPC providers #252

Conversation

rvanasa commented Aug 13, 2024 • edited Loading

rvanasa commented Aug 30, 2024

gregorydemay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvanasa Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvanasa Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

rvanasa commented Sep 5, 2024

rvanasa commented Aug 13, 2024 •

edited

Loading

rvanasa Sep 4, 2024 •

edited

Loading

rvanasa Sep 4, 2024 •

edited

Loading