Configure symbol groups by name #403

jayantk · 2022-11-28T19:13:52Z

This PR adds the ability to configure which symbols are in each group using just the name of the symbol. This change will make it easier for us to maintain the configuration file as we customize the groups.

I refactored the code a little to simplify the change. Specifically, the previous code used to read AttestationConfig at startup, then mutate it over time to store any additional symbols read from the mapping account. I refactored the code so there's a unidirectional information flow AttestationConfig -> merged with mapping data to create list of batches -> batches are published. I then extracted out the logic for merging the config with the mapping data into a function. Check out the first commit to see the general shape of the refactor.

See inline comments for added details on changes.

jayantk · 2022-11-28T19:38:28Z

pythnet/remote-executor/programs/remote-executor/src/lib.rs

@@ -1,5 +1,5 @@
 #![deny(warnings)]
-#![allow(clippy::result_large_err)]
+#![allow(clippy::result_unit_err)]


clippy changed this for some reason

jayantk · 2022-11-28T19:39:31Z

solana/pyth2wormhole/client/src/lib.rs

@@ -402,8 +399,8 @@ pub fn gen_attest_tx(
 pub async fn crawl_pyth_mapping(
    rpc_client: &RpcClient,
    first_mapping_addr: &Pubkey,
-) -> Result<HashMap<Pubkey, HashSet<Pubkey>>, ErrBox> {
-    let mut ret = HashMap::new();
+) -> Result<Vec<P2WProductAccount>, ErrBox> {


adding the name of each product to the result here (so we can match on it to make the batches)

jayantk · 2022-11-28T19:40:38Z

third_party/pyth/p2w_autoattest.py

+default_attestation_conditions:
+  min_interval_secs: 30
+  price_changed_bps: 500
+name_groups:


hacked the existing tests to include name groups

I don't think leaving this empty is right. I think Tilt setup should be able to show that the name group settings are respected by default. Other than that, we should be able to confirm that this setting is respected in the log, ideally with warn! on name groups that were never used.

jayantk · 2022-11-28T19:57:16Z

solana/pyth2wormhole/client/src/attestation_cfg.rs

+    pub min_rpc_interval_ms:            u64,
+    /// Attestation conditions that will be used for any symbols included in the mapping
+    /// that aren't explicitly in one of the groups below.
+    pub default_attestation_conditions: AttestationConditions,


the old code could use AttestationConditions::default() for these symbols under some conditions (if the config file didn't have a "mapping" group). That old behavior is likely to lead to configuration bugs, so I fixed it here.

I think the comment is a bit misleading here. It also adds the condition to named groups.

I think it's even better to add this to the symbol groups as well.

solana/pyth2wormhole/client/src/main.rs

solana/pyth2wormhole/client/src/attestation_cfg.rs

solana/pyth2wormhole/client/src/main.rs

ali-behjati

The refactoring and the change set looks great! I left some minor comments about the code.

Just one big concern about testing the code, because afaik we do not add symbol in the pyth_publisher.py and the returned name (that you used) is just a debug information. The e2e test is not failing because the current logic skips the symbols. To add symbol name to a product (and fix testing) you will need to call pyth_admin upd_price like this test.

Second comment is that do you see any use case for keeping the symbol_groups approach? I am in favor of removing it entirely (to keep the code simple).

drozdziak1

I value the use case but I am not sold on the scope of this changeset. IIUC the intent is to add the ability to say "Any symbol name containing "BTC" lands here" etc. I think this can be achieved without the need to create a whole new group type (one could slap a Vec<String> member on the existing group type instead) and especially without messing with the utility methods on the attestation config type. I do not agree with the unidirectional flow idea because I don't think the new utility functions have a meaningful life outside AttestationConfig. I think that dragging a batch vector outside attestation_config obfuscates the source of truth about available batches. Ultimately, I do not believe this change is complete without unit-testing the new config merging logic, descriptive logging and a convincing reason to widen the scope beyond just sorting the batches into their matching buckets.

solana/pyth2wormhole/client/src/main.rs

drozdziak1 · 2022-11-29T17:06:55Z

third_party/pyth/p2w_autoattest.py

+default_attestation_conditions:
+  min_interval_secs: 30
+  price_changed_bps: 500
+name_groups:


I don't think leaving this empty is right. I think Tilt setup should be able to show that the name group settings are respected by default. Other than that, we should be able to confirm that this setting is respected in the log, ideally with warn! on name groups that were never used.

solana/pyth2wormhole/client/src/main.rs

solana/pyth2wormhole/client/src/attestation_cfg.rs

jayantk · 2022-12-01T19:28:13Z

third_party/pyth/p2w_autoattest.py

@@ -137,6 +137,8 @@
 mapping_reload_interval_mins: 1 # Very fast for testing purposes
 min_rpc_interval_ms: 0 # RIP RPC
 max_batch_jobs: 1000 # Where we're going there's no oomkiller
+default_attestation_conditions:


@drozdziak1 and i agreed that i can update the integration tests in a follow-up, since we currently don't have a good way to validate that the things in the config are actually being attested.

ali-behjati

Thanks for addressing my comments. Looking forward to adding it to the tests. I like to revisit keeping the old configuration way after adding proper e2e tests.

drozdziak1

Nice! I like the new lookup/symbol merging separation. My remaining concerns are primarily naming, comments and some edge cases.

drozdziak1 · 2022-12-02T14:11:46Z

solana/pyth2wormhole/client/src/lib.rs

@@ -439,6 +435,13 @@ pub async fn crawl_pyth_mapping(
                }
            };

+            let mut prod_name = "";


Can we default to "<unspecified>" or similar? Extra credit for an Option<>. If our rules parsing grows, we may get a nasty edge case with something.contains("") which is always true for any string

drozdziak1 · 2022-12-02T14:15:46Z

solana/pyth2wormhole/client/src/main.rs

+        vec![]
+    };
+
+    Ok(attestation_cfg.instantiate_batches(&products, max_batch_size))


Nice! The mapping lookup/config merge divide is way more visible

drozdziak1 · 2022-12-02T14:24:17Z

solana/pyth2wormhole/client/src/main.rs

@@ -510,22 +504,43 @@ async fn handle_attest_non_daemon_mode(
    Ok(())
 }

+async fn attestation_config_to_batches(


Can you add a comment about how this works? i.e. that we do an on-chain lookup and we merge results with attestation config and then collapse everything to a vec? The on-chain state lookup is not obvious at first glance. Ideally the function's name could conceive that, but I had no success coming up with anything less than 40 chars 😅

drozdziak1 · 2022-12-02T14:26:18Z

solana/pyth2wormhole/client/src/attestation_cfg.rs

+    pub max_msg_accounts:          u64,
+
+    /// Optionally, we take a mapping account to add remaining symbols from a Pyth deployments.
+    /// These symbols are processed under `default_attestation_conditions`.


I think this is no longer true (the group decides that, and the defaults only apply if no group matches the symbol)

drozdziak1 · 2022-12-02T14:28:07Z

solana/pyth2wormhole/client/src/attestation_cfg.rs

-                }
+    /// Instantiate the batches of symbols to attest by matching the config against the collection
+    /// of on-chain product accounts.
+    pub fn instantiate_batches(


I don't think the comment has to mention anything about on-chain. This just collapses the config into a vec and optionally merges in extra symbols

drozdziak1 · 2022-12-02T14:40:15Z

solana/pyth2wormhole/client/src/attestation_cfg.rs

+        max_batch_size: usize,
+    ) -> Vec<SymbolBatch> {
+        // Construct mapping from the name of each product account to its corresponding symbols
+        let mut name_to_symbols: HashMap<String, Vec<P2WSymbol>> = HashMap::new();


I think this will cause duplicate-named products in the mapping behave in a very ambiguous way. Such edge case would cause two distinct P2WProductAccounts. This is then obfuscated by putting symbols from both in the same bucket on the name_to_symbols hashmap. This has no serious consequences right now, but I think it's confusing.

drozdziak1 · 2022-12-02T14:46:32Z

solana/pyth2wormhole/client/src/attestation_cfg.rs

+    },
+    /// A symbol specified by its product and price account keys.
+    Key {
+        /// Optional human-readable name for the symbol (for logging purposes).


This is exactly the same as P2WSymbol, have you considered Key(P2WSymbol)?

drozdziak1 · 2022-12-02T14:47:38Z

solana/pyth2wormhole/client/src/attestation_cfg.rs

+/// Config entry for a symbol to attest.
+#[derive(Clone, Debug, Hash, Deserialize, Serialize, PartialEq, Eq)]
+#[serde(tag = "type", rename_all = "snake_case")]
+pub enum SymbolConfig {


I think it's more like a symbol reference than a config. You're not affecting symbol behavior here, only identifying them.

drozdziak1 · 2022-12-02T14:53:47Z

solana/pyth2wormhole/client/src/attestation_cfg.rs

+}
+
+#[derive(Clone, Debug, Hash, Deserialize, Serialize, PartialEq, Eq)]
+pub struct SymbolBatch {


Can you explain in a doc comment how the types are used? This one is easy to mistake with SymbolGroupConfig

drozdziak1 · 2022-12-02T15:01:09Z

solana/pyth2wormhole/client/src/attestation_cfg.rs

-            mapping_reload_interval_mins: 42,
-            symbol_groups:                vec![],
-        };
+    fn test_instantiate_batches() -> Result<(), ErrBox> {


This test does not assert desired behavior edge cases:

Group already has the same symbol

No name group matches this symbol (default group)

brand new symbol is added to the right place

Jayant Krishnamurthy added 13 commits November 23, 2022 09:38

Refactor to make this change easier

6e3f387

stop mutating AttestationConfig

93c45d4

get the product names also

a049df6

ok

0b28a52

refactor

9627bce

cleanup

daca4dc

more cleanup

c88a7e0

more cleanup

f036c09

comment

20fa60f

i think this works

3aa4d3f

merge

7d5cfd6

fix stuff

d707f56

clippy

2017115

jayantk commented Nov 28, 2022

View reviewed changes

ali-behjati reviewed Nov 29, 2022

View reviewed changes

solana/pyth2wormhole/client/src/main.rs Outdated Show resolved Hide resolved

ali-behjati reviewed Nov 29, 2022

View reviewed changes

solana/pyth2wormhole/client/src/main.rs Outdated Show resolved Hide resolved

ali-behjati reviewed Nov 29, 2022

View reviewed changes

solana/pyth2wormhole/client/src/attestation_cfg.rs Outdated Show resolved Hide resolved

ali-behjati reviewed Nov 29, 2022

View reviewed changes

solana/pyth2wormhole/client/src/attestation_cfg.rs Outdated Show resolved Hide resolved

ali-behjati reviewed Nov 29, 2022

View reviewed changes

solana/pyth2wormhole/client/src/main.rs Outdated Show resolved Hide resolved

ali-behjati reviewed Nov 29, 2022

View reviewed changes

drozdziak1 suggested changes Nov 29, 2022

View reviewed changes

drozdziak1 reviewed Nov 29, 2022

View reviewed changes

solana/pyth2wormhole/client/src/attestation_cfg.rs Show resolved Hide resolved

Jayant Krishnamurthy added 5 commits November 29, 2022 13:48

more cleanup

a8f60bf

main

efade98

main

f8d2653

fix formatting

c5af84a

blah

927cf59

Jayant Krishnamurthy added 8 commits December 1, 2022 07:59

test

1879610

cleanup

ac08f2e

fix python

1d4a611

Merge branch 'main' into config-improvements

63928c7

config

b025711

fix test

4ce9575

grr

90889e9

grr

caceedb

jayantk commented Dec 1, 2022

View reviewed changes

ali-behjati approved these changes Dec 2, 2022

View reviewed changes

drozdziak1 reviewed Dec 2, 2022

View reviewed changes

drozdziak1 self-requested a review December 2, 2022 15:46

drozdziak1 approved these changes Dec 2, 2022

View reviewed changes

comments

83f8502

jayantk merged commit 3beffdf into main Dec 2, 2022

jayantk deleted the config-improvements branch December 2, 2022 17:35

Configure symbol groups by name #403

Configure symbol groups by name #403

Uh oh!

Conversation

jayantk commented Nov 28, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ali-behjati left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drozdziak1 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ali-behjati left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drozdziak1 left a comment

Choose a reason for hiding this comment

Uh oh!

drozdziak1 Dec 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drozdziak1 Dec 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ali-behjati left a comment •

edited

Loading

drozdziak1 left a comment •

edited

Loading

ali-behjati left a comment •

edited

Loading

drozdziak1 Dec 2, 2022 •

edited

Loading

drozdziak1 Dec 2, 2022 •

edited

Loading