diff --git a/doc/code-guidelines.md b/doc/code-guidelines.md index 819a92c21c..08905b23ea 100644 --- a/doc/code-guidelines.md +++ b/doc/code-guidelines.md @@ -7,9 +7,9 @@ # Make your services as resilient to errors as possible - Perform more benchmarking/add benchmarking tests through our codebase. Currently -there are portions of the codebase that we have unknown performance for that -may become more important as we scale. Most languages have benchmark test -capability, rust has it built in for example. + there are portions of the codebase that we have unknown performance for that + may become more important as we scale. Most languages have benchmark test + capability, rust has it built in for example. - Implement error recovery even for unlikely cases. Think about how the service can continue to work after different failures. - The service should continue to work (if possible) if its dependencies are unavailable or broken. - Avoid the possibility of leaving the service in a state where it no longer able to start or work properly. The service should be able to recover from things like invalid files or unexpected database state. If that is not possible, provide clear error messages that explain what should be done to fix it. @@ -19,225 +19,225 @@ capability, rust has it built in for example. # Set up essential tooling - Use strongest lint settings. It is better to have at minimum pedantic warnings -on all projects. Good examples of bad settings: allowing `any` globally in -typescript, ignoring integer clippy type warnings in Rust, etc. + on all projects. Good examples of bad settings: allowing `any` globally in + typescript, ignoring integer clippy type warnings in Rust, etc. - Add extensive logging, metrics and tracing capability early, much of our code is missing -metrics, good log handling, or ability to do introspection on code that has -failed in retrospect. Good example: hermes launch. + metrics, good log handling, or ability to do introspection on code that has + failed in retrospect. Good example: hermes launch. # Keep the code readable and maintainable - Make heavy use of types to define behaviour. In general introducing a type can be -thought of as introducing a unit test. For example: + thought of as introducing a unit test. For example: - ```rust - struct PositiveTime(i64); + ```rust + struct PositiveTime(i64); - impl TryFrom for PositiveTime { - type Err = (); - fn try_from(n: i64) -> Result { - if n < 0 { - return Err(()); - } - return Ok(Self(n)); - } - } + impl TryFrom for PositiveTime { + type Err = (); + fn try_from(n: i64) -> Result { + if n < 0 { + return Err(()); + } + return Ok(Self(n)); + } + } - ``` + ``` - This can be thought of reducing the valid range of i64 to one we prefer - (given that i64 is the native Linux time type but often we do not want these) - that we can enforce a compile-time. The benefit in types over unit tests is - simply use-at-site of a type ensure behaviour everywhere and reducing the - amount of unwanted behaviour in a codebase. + This can be thought of reducing the valid range of i64 to one we prefer + (given that i64 is the native Linux time type but often we do not want these) + that we can enforce a compile-time. The benefit in types over unit tests is + simply use-at-site of a type ensure behaviour everywhere and reducing the + amount of unwanted behaviour in a codebase. - Currently we do not try hard enough to isolate behaviours through types. + Currently we do not try hard enough to isolate behaviours through types. - Avoid monolithic event handlers, and avoid state handling in logic. Some -stateful code in our repos mixes the logic handling with the state handle -code which produces very long, hard to reason about code which ends up as -a rather large inline state machine: + stateful code in our repos mixes the logic handling with the state handle + code which produces very long, hard to reason about code which ends up as + a rather large inline state machine: - Good: + Good: - ```tsx - function handleEvent(e, state) { - switch(e.type) { - case Event.Websocket: handleWebsocketEvent(e, state.websockets); - case Event.PythNet: handlePythnetEvent(e, state.pyth_handle); - case ... - } - } + ```tsx + function handleEvent(e, state) { + switch(e.type) { + case Event.Websocket: handleWebsocketEvent(e, state.websockets); + case Event.PythNet: handlePythnetEvent(e, state.pyth_handle); + case ... + } + } - ``` + ``` - Bad: + Bad: - ```tsx - function handleEvent(e) { - // Many inlined state tracking vars. Not much better than globals. - var latstPythNetupdateTime = DateTime.now(); - var clientsWaiting = {}; - var ... + ```tsx + function handleEvent(e) { + // Many inlined state tracking vars. Not much better than globals. + var latstPythNetupdateTime = DateTime.now(); + var clientsWaiting = {}; + var ... - switch(e.type) { - // lots of inline handling - } - } + switch(e.type) { + // lots of inline handling + } + } - ``` + ``` - Avoid catch-all modules, I.E: `types/`, `utils/` - Favor Immutability and Idempotency. Both are a huge source of reducing logic bugs. - State should whenever possible flow top-down, I.E: create at entry point and -flow to other components. Global state should be avoided and no state should be -hidden in separate modules. + flow to other components. Global state should be avoided and no state should be + hidden in separate modules. - Good: + Good: - ```tsx - // main.ts - function main() { - const db = db.init(); - initDb(db); - } + ```tsx + // main.ts + function main() { + const db = db.init(); + initDb(db); + } - ``` + ``` - Bad: + Bad: - ```tsx - // main.ts - const { db } = require('db'); - function() { - initDb(); // Databaes not passed, implies global use. - } + ```tsx + // main.ts + const { db } = require('db'); + function() { + initDb(); // Databaes not passed, implies global use. + } - ``` + ``` - For types/functions that are only used once, keep them close to the -definition. If they are re-used, try and lift them only up to a common -parent, in the following example types/functions only lift as far -as they are useful: + definition. If they are re-used, try and lift them only up to a common + parent, in the following example types/functions only lift as far + as they are useful: - Example File Hierarchy: + Example File Hierarchy: - ``` - lib/routes.rs:validateUserId() - lib/routes/user.rs:type RequestUser - lib/routes/user/register.rs:generateRandomUsername() + ``` + lib/routes.rs:validateUserId() + lib/routes/user.rs:type RequestUser + lib/routes/user/register.rs:generateRandomUsername() - ``` + ``` - Good: + Good: - ```tsx - // Definition only applies to this function, keep locality. - type FeedResponse = { - id: FeedId, - feed: Feed, - }; + ```tsx + // Definition only applies to this function, keep locality. + type FeedResponse = { + id: FeedId, + feed: Feed, + }; - // Note the distinction between FeedResponse/Feed for DDD. - function getFeed(id: FeedId, db: Db): FeedResponse { - let feed: Feed = db.execute(FEED_QUERY, [id]); - return { id, feed: feed, } - } + // Note the distinction between FeedResponse/Feed for DDD. + function getFeed(id: FeedId, db: Db): FeedResponse { + let feed: Feed = db.execute(FEED_QUERY, [id]); + return { id, feed: feed, } + } - ``` + ``` - Bad: + Bad: - ```tsx - import { FeedResponse } from 'types'; - function getFeed(id: FeedId, db: Db): FeedResponse { - let feed = db.execute(FEED_QUERY, [id]); - return { id, feed: feed, } - } + ```tsx + import { FeedResponse } from 'types'; + function getFeed(id: FeedId, db: Db): FeedResponse { + let feed = db.execute(FEED_QUERY, [id]); + return { id, feed: feed, } + } - ``` + ``` - Map functionality into submodules when a module defines a category of handlers. -This help emphasise where code re-use should happen, for example: + This help emphasise where code re-use should happen, for example: - Good: + Good: - ``` - src/routes/user/register.ts - src/routes/user/login.ts - src/routes/user/add_role.ts - src/routes/index.ts + ``` + src/routes/user/register.ts + src/routes/user/login.ts + src/routes/user/add_role.ts + src/routes/index.ts - ``` + ``` - Bad: + Bad: - ```tsx - // src/index.ts - function register() { ... } - function login() { ... } - function addRole() { ... } - function index() { ... } + ```tsx + // src/index.ts + function register() { ... } + function login() { ... } + function addRole() { ... } + function index() { ... } - ``` + ``` - Not only does this make large unwieldy files but it encourages things like - `types/` catch alls, or unnecessary sharing of functionality. For example - imagine a `usernameAsBase58` function thrown into this file, that then - looks useful within an unrelated to users function, it can be tempting to - abuse the utility function or move it to a vague catch-all location. Focus - on clear, API boundaries even within our own codebase. + Not only does this make large unwieldy files but it encourages things like + `types/` catch alls, or unnecessary sharing of functionality. For example + imagine a `usernameAsBase58` function thrown into this file, that then + looks useful within an unrelated to users function, it can be tempting to + abuse the utility function or move it to a vague catch-all location. Focus + on clear, API boundaries even within our own codebase. - When possible use layered architecture (onion/hexagonal/domain driven design) where -we separate API processing, business logic, and data logic. The benefit of this -is it defines API layers within the application itself: - - Good: - - ```tsx - // web/user/register.ts - import { registerUser, User } from 'api/user/register.ts'; - - // Note locality: one place use functions stay near, no utils/ - function verifyUsername( ... - function verifyPassword( ... - - // Locality again. - type RegisterRequest = { - ... - }; - - function register(req: RegisterRequest): void { - // Validation Logic Only - verifyUsername(req.username); - verifyPassword(req.password); - - // Business Logic Separate - registerUser({ - username: req.username, - password: req.password, - }); - } - - ``` - - ```tsx - // api/user/register.ts - import { storeUser, DbUser } from 'db/user'; - - function registerUser(user: User) { - const user = fetchByUsername(user.username); - if (user) { - throw "User Exists; - } - - // Note again that the type used here differs from User (DbUser) which - // prevents code breakage (such as if the schema is updated but the - // code is not. - storeUser({ - username: user.username, - password: hash(user.password), - }); - } - - ``` + we separate API processing, business logic, and data logic. The benefit of this + is it defines API layers within the application itself: + + Good: + + ```tsx + // web/user/register.ts + import { registerUser, User } from 'api/user/register.ts'; + + // Note locality: one place use functions stay near, no utils/ + function verifyUsername( ... + function verifyPassword( ... + + // Locality again. + type RegisterRequest = { + ... + }; + + function register(req: RegisterRequest): void { + // Validation Logic Only + verifyUsername(req.username); + verifyPassword(req.password); + + // Business Logic Separate + registerUser({ + username: req.username, + password: req.password, + }); + } + + ``` + + ```tsx + // api/user/register.ts + import { storeUser, DbUser } from 'db/user'; + + function registerUser(user: User) { + const user = fetchByUsername(user.username); + if (user) { + throw "User Exists; + } + + // Note again that the type used here differs from User (DbUser) which + // prevents code breakage (such as if the schema is updated but the + // code is not. + storeUser({ + username: user.username, + password: hash(user.password), + }); + } + + ``` diff --git a/doc/rust-code-guidelines.md b/doc/rust-code-guidelines.md index 0dfe48ce4d..dbbeacf90a 100644 --- a/doc/rust-code-guidelines.md +++ b/doc/rust-code-guidelines.md @@ -14,48 +14,47 @@ Note: general [Code Guidelines](code-guidelines.md) also apply. - (expand to see clippy configuration) - ```toml - [lints.rust] - unsafe_code = "deny" - - [lints.clippy] - wildcard_dependencies = "deny" - - collapsible_if = "allow" - collapsible_else_if = "allow" - - allow_attributes_without_reason = "warn" - - # Panics - expect_used = "warn" - fallible_impl_from = "warn" - indexing_slicing = "warn" - panic = "warn" - panic_in_result_fn = "warn" - string_slice = "warn" - todo = "warn" - unchecked_duration_subtraction = "warn" - unreachable = "warn" - unwrap_in_result = "warn" - unwrap_used = "warn" - - # Correctness - cast_lossless = "warn" - cast_possible_truncation = "warn" - cast_possible_wrap = "warn" - cast_sign_loss = "warn" - collection_is_never_read = "warn" - match_wild_err_arm = "warn" - path_buf_push_overwrite = "warn" - read_zero_byte_vec = "warn" - same_name_method = "warn" - suspicious_operation_groupings = "warn" - suspicious_xor_used_as_pow = "warn" - unused_self = "warn" - used_underscore_binding = "warn" - while_float = "warn" - ``` - + ```toml + [lints.rust] + unsafe_code = "deny" + + [lints.clippy] + wildcard_dependencies = "deny" + + collapsible_if = "allow" + collapsible_else_if = "allow" + + allow_attributes_without_reason = "warn" + + # Panics + expect_used = "warn" + fallible_impl_from = "warn" + indexing_slicing = "warn" + panic = "warn" + panic_in_result_fn = "warn" + string_slice = "warn" + todo = "warn" + unchecked_duration_subtraction = "warn" + unreachable = "warn" + unwrap_in_result = "warn" + unwrap_used = "warn" + + # Correctness + cast_lossless = "warn" + cast_possible_truncation = "warn" + cast_possible_wrap = "warn" + cast_sign_loss = "warn" + collection_is_never_read = "warn" + match_wild_err_arm = "warn" + path_buf_push_overwrite = "warn" + read_zero_byte_vec = "warn" + same_name_method = "warn" + suspicious_operation_groupings = "warn" + suspicious_xor_used_as_pow = "warn" + unused_self = "warn" + used_underscore_binding = "warn" + while_float = "warn" + ``` The recommendations on this page should help with dealing with these lints. @@ -92,9 +91,10 @@ Panics are dangerous because they can arise from concise code constructions (suc **Common sources of panics:** | Source of panic | Non-panicking alternatives | -| --- | --- | +| --------------- | -------------------------- | + | `.unwrap()`, `.expect(...)`, -`panic!()`, `assert*!()` | `Result`-based handling using `anyhow` crate: +`panic!()`, `assert*!()` | `Result`-based handling using `anyhow` crate: `.context()?` , `.with_context()?` , `bail!()`, `ensure!()`. | | `unimplemented!()`, `todo!()`, `unreachable!()` | β€” | @@ -103,7 +103,7 @@ Panics are dangerous because they can arise from concise code constructions (suc | Indexing a `HashMap` or `BTreeMap` on a non-existing key: `&map[key]` | `.get()`, `.get_mut()`, `.entry()` | | Indexing a non-ASCII string not at char boundaries: `&"😒😒😒"[0..1]` | `.get()`, `.get_mut()` | | `Vec` methods: `.insert()`, `.remove()`, `.drain()`, `.split_off()` and many more | There are checked alternatives for some but not all of them. | -| Division by zero: `a / b`, `a % b` | `.checked_div()`, `.checked_rem()`, `/ NonZero*` | +| Division by zero: `a / b`, `a % b` | `.checked_div()`, `.checked_rem()`, `/ NonZero*` | Think about the cases that could cause your code to panic. Try to rewrite the code in a way that avoids a possible panic. @@ -111,239 +111,238 @@ Here are some tips to solve `clippy` warnings and avoid panics: - **Use `Result` type and return the error to the caller.** Use `.context()` or `.with_context()` from `anyhow` crate to add relevant information to the error. Use `.ok_or_else()` or `.context()` to convert `Option` to `Result`. - β›” Bad: + β›” Bad: - ```rust - pub fn best_bid(response: &Response) -> String { - response.bids[0].clone() - } - ``` + ```rust + pub fn best_bid(response: &Response) -> String { + response.bids[0].clone() + } + ``` - βœ… Good: + βœ… Good: - ```rust - pub fn best_bid(response: &Response) -> anyhow::Result { - Ok(response.bids.first().context("expected 1 item in bids, got 0")?.clone()) - } - ``` + ```rust + pub fn best_bid(response: &Response) -> anyhow::Result { + Ok(response.bids.first().context("expected 1 item in bids, got 0")?.clone()) + } + ``` - **If propagation with `?` doesn’t work because of error type, convert the error type with `.map_err()`.** - βœ… Good (`binary_search` returns `Err(index)` instead of a well-formed error, so we create our own error value): + βœ… Good (`binary_search` returns `Err(index)` instead of a well-formed error, so we create our own error value): - ```rust - items - .binary_search(&needle) - .map_err(|_| anyhow!("item not found: {:?}", needle))?; - ``` + ```rust + items + .binary_search(&needle) + .map_err(|_| anyhow!("item not found: {:?}", needle))?; + ``` - βœ… Good (`?` can’t convert from `Box` to `anyhow::Error`, but there is a function that converts it): + βœ… Good (`?` can’t convert from `Box` to `anyhow::Error`, but there is a function that converts it): - ```rust - let info = message.info().map_err(anyhow::Error::from_boxed)?; - ``` + ```rust + let info = message.info().map_err(anyhow::Error::from_boxed)?; + ``` - **If the error is in a non-Result function inside an iterator chain (e.g. `.map()`) or a combinator (e.g. `.unwrap_or_else()`), consider rewriting it as a plain `for` loop or `match`/`if let` to allow error propagation.** (If you’re determined to make iterators work, `fallible-iterator` crate may also be useful.) - β›” Bad (unwraps the error just because `?` doesn’t work): - - ```rust - fn check_files(paths: &[&Path]) -> anyhow::Result<()> { - let good_paths: Vec<&Path> = paths - .iter() - .copied() - .filter(|path| fs::read_to_string(path).unwrap().contains("magic")) - .collect(); - //... - } - ``` - - βœ… Good: - - ```rust - fn check_files(paths: &[&Path]) -> anyhow::Result<()> { - let mut good_paths = Vec::new(); - for path in paths { - if fs::read_to_string(path)?.contains("magic") { - good_paths.push(path); - } - } - //... - } - ``` + β›” Bad (unwraps the error just because `?` doesn’t work): + + ```rust + fn check_files(paths: &[&Path]) -> anyhow::Result<()> { + let good_paths: Vec<&Path> = paths + .iter() + .copied() + .filter(|path| fs::read_to_string(path).unwrap().contains("magic")) + .collect(); + //... + } + ``` + + βœ… Good: + + ```rust + fn check_files(paths: &[&Path]) -> anyhow::Result<()> { + let mut good_paths = Vec::new(); + for path in paths { + if fs::read_to_string(path)?.contains("magic") { + good_paths.push(path); + } + } + //... + } + ``` - **Log the error and return early or skip an item.** - β›” Bad (panics if we add too many publishers): - - ```rust - let publisher_count = u16::try_from(prices.len()) - .expect("too many publishers"); - ``` - - βœ… Good: - - ```rust - let Ok(publisher_count) = u16::try_from(prices.len()) else { - error!("too many publishers ({})", prices.len()); - return Default::default(); - }; - ``` - - β›” Bad (terminates on error) (and yes, [it can fail](https://www.greyblake.com/blog/when-serde-json-to-string-fails/)): - - ```rust - loop { - //... - yield Ok( - serde_json::to_string(&response).expect("should not fail") + "\n" - ); - } - ``` - - βœ… Good (`return` instead of `continue` could also be reasonable): - - ```rust - loop { - //... - match serde_json::to_string(&response) { - Ok(json) => { - yield Ok(json + "\n"); - } - Err(err) => { - error!("json serialization error for {:?}: {}", response, err); - continue; - } - } - } - ``` + β›” Bad (panics if we add too many publishers): + + ```rust + let publisher_count = u16::try_from(prices.len()) + .expect("too many publishers"); + ``` + + βœ… Good: + + ```rust + let Ok(publisher_count) = u16::try_from(prices.len()) else { + error!("too many publishers ({})", prices.len()); + return Default::default(); + }; + ``` + + β›” Bad (terminates on error) (and yes, [it can fail](https://www.greyblake.com/blog/when-serde-json-to-string-fails/)): + + ```rust + loop { + //... + yield Ok( + serde_json::to_string(&response).expect("should not fail") + "\n" + ); + } + ``` + + βœ… Good (`return` instead of `continue` could also be reasonable): + + ```rust + loop { + //... + match serde_json::to_string(&response) { + Ok(json) => { + yield Ok(json + "\n"); + } + Err(err) => { + error!("json serialization error for {:?}: {}", response, err); + continue; + } + } + } + ``` - **Supply a sensible default:** - β›” Bad: + β›” Bad: - ```rust - let interval_us: u64 = snapshot_interval - .as_micros() - .try_into() - .expect("snapshot_interval overflow"); - ``` - - βœ… Better: - - ```rust - let interval_us = - u64::try_from(snapshot_interval.as_micros()).unwrap_or_else(|_| { - error!( - "invalid snapshot_interval in config: {:?}, defaulting to 1 min", - snapshot_interval - ); - 60_000_000 // 1 min - }); - ``` + ```rust + let interval_us: u64 = snapshot_interval + .as_micros() + .try_into() + .expect("snapshot_interval overflow"); + ``` + + βœ… Better: + + ```rust + let interval_us = + u64::try_from(snapshot_interval.as_micros()).unwrap_or_else(|_| { + error!( + "invalid snapshot_interval in config: {:?}, defaulting to 1 min", + snapshot_interval + ); + 60_000_000 // 1 min + }); + ``` - **Avoid checking a condition and then unwrapping.** Instead, combine the check and access to the value using `match`, `if let`, and combinators such as `map_or`, `some_or`, etc. - 🟑 Not recommended: + 🟑 Not recommended: - ```rust - if !values.is_empty() { - process_one(&values[0]); - } - ``` + ```rust + if !values.is_empty() { + process_one(&values[0]); + } + ``` - βœ… Good: + βœ… Good: - ```rust - if let Some(first_value) = values.first() { - process_one(first_value); - } - ``` + ```rust + if let Some(first_value) = values.first() { + process_one(first_value); + } + ``` - 🟑 Not recommended: + 🟑 Not recommended: - ```rust - .filter(|price| { - median_price.is_none() || price < &median_price.expect("should not fail") - }) - ``` + ```rust + .filter(|price| { + median_price.is_none() || price < &median_price.expect("should not fail") + }) + ``` - βœ… Good: + βœ… Good: - ```rust - .filter(|price| median_price.is_none_or(|median_price| price < &median_price)) - ``` + ```rust + .filter(|price| median_price.is_none_or(|median_price| price < &median_price)) + ``` - 🟑 Not recommended: + 🟑 Not recommended: - ```rust - if data.len() < header_len { - bail!("data too short"); - } - let header = &data[..header_len]; - let payload = &data[header_len..]; - ``` + ```rust + if data.len() < header_len { + bail!("data too short"); + } + let header = &data[..header_len]; + let payload = &data[header_len..]; + ``` - βœ… Good: + βœ… Good: - ```rust - let (header, payload) = data - .split_at_checked(header_len) - .context("data too short")?; - ``` + ```rust + let (header, payload) = data + .split_at_checked(header_len) + .context("data too short")?; + ``` - **Avoid the panic by introducing a constant or move the panic inside the constant initialization.** Panicking in const initializers is safe because it happens at compile time. - 🟑 Not recommended: + 🟑 Not recommended: - ```rust - price.unwrap_or(Price::new(PRICE_FEED_EPS).expect("should never fail")) - ``` + ```rust + price.unwrap_or(Price::new(PRICE_FEED_EPS).expect("should never fail")) + ``` - βœ… Good: + βœ… Good: - ```rust - #[allow(clippy::unwrap_used, reason = "safe in const")] - const MIN_POSITIVE_PRICE: Price = - Price(NonZeroI64::new(PRICE_FEED_EPS).unwrap()); - ``` + ```rust + #[allow(clippy::unwrap_used, reason = "safe in const")] + const MIN_POSITIVE_PRICE: Price = + Price(NonZeroI64::new(PRICE_FEED_EPS).unwrap()); + ``` - **If it’s not possible to refactor the code, allow the lint and specify the reason why the code cannot fail. Prefer `.expect()` over `.unwrap()` and specify the failure in the argument.** This is reasonable if the failure is truly impossible or if a failure would be so critical that the service cannot continue working. - 🟑 Not recommended: - - ```rust - Some(prices[prices.len() / 2]) - ``` + 🟑 Not recommended: - βœ… Good: + ```rust + Some(prices[prices.len() / 2]) + ``` - ```rust - #[allow( - clippy::indexing_slicing, - reason = "prices are not empty, prices.len() / 2 < prices.len()" - )] - Some(prices[prices.len() / 2]) - ``` + βœ… Good: - 🟑 Not recommended: + ```rust + #[allow( + clippy::indexing_slicing, + reason = "prices are not empty, prices.len() / 2 < prices.len()" + )] + Some(prices[prices.len() / 2]) + ``` - ```rust - let expiry_time = expiry_times.get(self.active_index).unwrap(); - ``` + 🟑 Not recommended: - βœ… Better: + ```rust + let expiry_time = expiry_times.get(self.active_index).unwrap(); + ``` - ```rust - #[allow( - clippy::expect_used, - reason = "we only assign valid indexes to `self.active_index`" - )] - let expiry_time = expiry_times - .get(self.active_index) - .expect("invalid active index"); - ``` + βœ… Better: + ```rust + #[allow( + clippy::expect_used, + reason = "we only assign valid indexes to `self.active_index`" + )] + let expiry_time = expiry_times + .get(self.active_index) + .expect("invalid active index"); + ``` # Avoid unchecked arithmetic operations @@ -382,7 +381,7 @@ You can catch unchecked arithmetic operations with `clippy::arithmetic_side_effe # Avoid implicit wrapping and truncation with `as` -Limit the use of `as` keyword for converting values between numeric types. While `as` is often the most convenient option, it’s quite bad at expressing intent. `as` can do conversions with different semantics. In the case of integers, it can do both **lossless conversions** (e.g. from `u8` to `u32`; from `u32` to `i64`) and **lossy conversions** (e.g. `258_u32 as u8` evaluates to 2; `200_u8 as i8` evaluates to -56). +Limit the use of `as` keyword for converting values between numeric types. While `as` is often the most convenient option, it’s quite bad at expressing intent. `as` can do conversions with different semantics. In the case of integers, it can do both **lossless conversions** (e.g. from `u8` to `u32`; from `u32` to `i64`) and **lossy conversions** (e.g. `258_u32 as u8` evaluates to 2; `200_u8 as i8` evaluates to -56). - Always use `.into()` and `T::from()` instead of `as` for lossless conversions. This makes the intent more clear. - Only use `as` for lossy conversions when you specifically intend to perform a lossy conversion and are aware of how it affects the value. Add an `#[allow]` attribute and specify the reason. @@ -433,81 +432,80 @@ Other recommendations: - The error message should contain relevant context that can help understand the issue. - β›” Bad: + β›” Bad: - ```rust - let timestamp = args.timestamp.try_into()?; - ``` + ```rust + let timestamp = args.timestamp.try_into()?; + ``` - βœ… Good (using `anyhow::Context`): + βœ… Good (using `anyhow::Context`): - ```rust - let timestamp = args - .timestamp - .try_into() - .with_context(|| { - format!("invalid timestamp received from Hermes: {:?}", args.timestamp) - })?; - ``` + ```rust + let timestamp = args + .timestamp + .try_into() + .with_context(|| { + format!("invalid timestamp received from Hermes: {:?}", args.timestamp) + })?; + ``` - β›” Bad: + β›” Bad: - ```rust - let exponent = config.feeds.get(feed_id).context("missing feed")?.exponent; - ``` + ```rust + let exponent = config.feeds.get(feed_id).context("missing feed")?.exponent; + ``` - βœ… Good: + βœ… Good: - ```rust - let exponent = config - .feeds - .get(feed_id) - .with_context(|| { - format!("missing feed {:?} in config", feed_id) - })? - .exponent; - ``` + ```rust + let exponent = config + .feeds + .get(feed_id) + .with_context(|| { + format!("missing feed {:?} in config", feed_id) + })? + .exponent; + ``` - When wrapping another error or converting error type, preserve the error source instead of converting it to `String`. Especially avoid calling `to_string()` on `anyhow::Error` or formatting it with `{}` because it erases context and backtrace information. - β›” Bad (loses information about source errors and backtrace): - - ```rust - if let Err(err) = parse(data) { - bail!("parsing failed: {err}"); - } - // OR - parse(data).map_err(|err| format!("parsing failed: {err}"))?; - ``` + β›” Bad (loses information about source errors and backtrace): - β›” Better but still bad (preserves source errors and backtrace, but the error will have two backtraces now): + ```rust + if let Err(err) = parse(data) { + bail!("parsing failed: {err}"); + } + // OR + parse(data).map_err(|err| format!("parsing failed: {err}"))?; + ``` - ```rust - if let Err(err) = parse(data) { - bail!("parsing failed for {data:?}: {err:?}"); - } - ``` + β›” Better but still bad (preserves source errors and backtrace, but the error will have two backtraces now): - βœ… Good (returns an error with proper source and backtrace): + ```rust + if let Err(err) = parse(data) { + bail!("parsing failed for {data:?}: {err:?}"); + } + ``` - ```rust - parse(data).with_context(|| format!("failed to parse {data:?}"))?; - ``` + βœ… Good (returns an error with proper source and backtrace): - β›” Bad (loses information about source errors and backtrace): + ```rust + parse(data).with_context(|| format!("failed to parse {data:?}"))?; + ``` - ```rust - warn!(%err, ?data, "parsing failed"); - // OR - warn!("parsing failed: {}", err); - ``` + β›” Bad (loses information about source errors and backtrace): - βœ… Better (returns full information about the error in a structured way): + ```rust + warn!(%err, ?data, "parsing failed"); + // OR + warn!("parsing failed: {}", err); + ``` - ```rust - warn!(?err, ?data, "parsing failed"); - ``` + βœ… Better (returns full information about the error in a structured way): + ```rust + warn!(?err, ?data, "parsing failed"); + ``` # Other recommendations diff --git a/governance/xc_admin/packages/xc_admin_cli/src/index.ts b/governance/xc_admin/packages/xc_admin_cli/src/index.ts index 8d28adaa65..1df4aaecd2 100644 --- a/governance/xc_admin/packages/xc_admin_cli/src/index.ts +++ b/governance/xc_admin/packages/xc_admin_cli/src/index.ts @@ -57,7 +57,7 @@ import { } from "@pythnetwork/pyth-solana-receiver"; import { SOLANA_LAZER_PROGRAM_ID, - SOLANA_STORAGE_ID, + SOLANA_LAZER_STORAGE_ID, } from "@pythnetwork/pyth-lazer-sdk"; import { LedgerNodeWallet } from "./ledger"; @@ -978,7 +978,7 @@ multisigCommand( .update(trustedSigner, expiryTime) .accounts({ topAuthority: await vault.getVaultAuthorityPDA(targetCluster), - storage: SOLANA_STORAGE_ID, + storage: new PublicKey(SOLANA_LAZER_STORAGE_ID), }) .instruction(); @@ -1010,7 +1010,7 @@ multisigCommand( const trustedSigner = Buffer.from(options.signer, "hex"); const expiryTime = new BN(options.expiryTime); - const programId = SOLANA_LAZER_PROGRAM_ID; + const programId = new PublicKey(SOLANA_LAZER_PROGRAM_ID); const programDataAccount = PublicKey.findProgramAddressSync( [programId.toBuffer()], BPF_UPGRADABLE_LOADER, @@ -1039,7 +1039,7 @@ multisigCommand( // Create Anchor program instance const lazerProgram = new Program( lazerIdl as Idl, - SOLANA_LAZER_PROGRAM_ID, + programId, vault.getAnchorProvider(), ); @@ -1048,7 +1048,7 @@ multisigCommand( .updateEcdsaSigner(trustedSigner, expiryTime) .accounts({ topAuthority: await vault.getVaultAuthorityPDA(targetCluster), - storage: SOLANA_STORAGE_ID, + storage: new PublicKey(SOLANA_LAZER_STORAGE_ID), }) .instruction(); diff --git a/governance/xc_admin/packages/xc_admin_common/src/multisig_transaction/index.ts b/governance/xc_admin/packages/xc_admin_common/src/multisig_transaction/index.ts index 445b4575d2..007ffb6eb2 100644 --- a/governance/xc_admin/packages/xc_admin_common/src/multisig_transaction/index.ts +++ b/governance/xc_admin/packages/xc_admin_common/src/multisig_transaction/index.ts @@ -166,7 +166,9 @@ export class MultisigParser { return SolanaStakingMultisigInstruction.fromTransactionInstruction( instruction, ); - } else if (instruction.programId.equals(SOLANA_LAZER_PROGRAM_ID)) { + } else if ( + instruction.programId.equals(new PublicKey(SOLANA_LAZER_PROGRAM_ID)) + ) { return LazerMultisigInstruction.fromInstruction(instruction); } else { return UnrecognizedProgram.fromTransactionInstruction(instruction); diff --git a/lazer/contracts/solana/package.json b/lazer/contracts/solana/package.json index 1fcd44cc29..f64f758658 100644 --- a/lazer/contracts/solana/package.json +++ b/lazer/contracts/solana/package.json @@ -10,7 +10,10 @@ "check-trusted-signer": "pnpm ts-node scripts/check_trusted_signer.ts" }, "dependencies": { - "@coral-xyz/anchor": "^0.30.1" + "@coral-xyz/anchor": "^0.30.1", + "@pythnetwork/pyth-lazer-sdk": "workspace:*", + "@solana/web3.js": "^1.98.0", + "@solana/buffer-layout": "^4.0.1" }, "devDependencies": { "@types/bn.js": "^5.1.0", diff --git a/lazer/contracts/solana/programs/pyth-lazer-solana-contract/src/lib.rs b/lazer/contracts/solana/programs/pyth-lazer-solana-contract/src/lib.rs index 85dbe59c48..c92e7fddc0 100644 --- a/lazer/contracts/solana/programs/pyth-lazer-solana-contract/src/lib.rs +++ b/lazer/contracts/solana/programs/pyth-lazer-solana-contract/src/lib.rs @@ -1,3 +1,5 @@ +#![allow(unexpected_cfgs)] // anchor macro triggers it + mod signature; use { diff --git a/lazer/contracts/solana/scripts/add_ed25519_signer.ts b/lazer/contracts/solana/scripts/add_ed25519_signer.ts new file mode 100644 index 0000000000..147efbe65b --- /dev/null +++ b/lazer/contracts/solana/scripts/add_ed25519_signer.ts @@ -0,0 +1,50 @@ +import * as anchor from "@coral-xyz/anchor"; +import { Program } from "@coral-xyz/anchor"; +import { PythLazerSolanaContract } from "../target/types/pyth_lazer_solana_contract"; +import * as pythLazerSolanaContractIdl from "../target/idl/pyth_lazer_solana_contract.json"; +import yargs from "yargs/yargs"; +import { readFileSync } from "fs"; +import NodeWallet from "@coral-xyz/anchor/dist/cjs/nodewallet"; + +// Add a trusted signer or change its expiry time. +// +// Example: +// pnpm ts-node scripts/add_ed25519_signer.ts --url 'https://api.testnet.solana.com' \ +// --keypair-path .../key.json --trusted-signer HaXscpSUcbCLSnPQB8Z7H6idyANxp1mZAXTbHeYpfrJJ \ +// --expiry-time-seconds 2057930841 +async function main() { + let argv = await yargs(process.argv.slice(2)) + .options({ + url: { type: "string", demandOption: true }, + "keypair-path": { type: "string", demandOption: true }, + "trusted-signer": { type: "string", demandOption: true }, + "expiry-time-seconds": { type: "number", demandOption: true }, + }) + .parse(); + + const keypair = anchor.web3.Keypair.fromSecretKey( + new Uint8Array(JSON.parse(readFileSync(argv.keypairPath, "ascii"))), + ); + + const wallet = new NodeWallet(keypair); + const connection = new anchor.web3.Connection(argv.url, { + commitment: "confirmed", + }); + const provider = new anchor.AnchorProvider(connection, wallet); + + const program: Program = new Program( + pythLazerSolanaContractIdl as PythLazerSolanaContract, + provider, + ); + + await program.methods + .update( + new anchor.web3.PublicKey(argv.trustedSigner), + new anchor.BN(argv.expiryTimeSeconds), + ) + .accounts({}) + .rpc(); + console.log("signer updated"); +} + +main(); diff --git a/lazer/contracts/solana/scripts/check_trusted_signer.ts b/lazer/contracts/solana/scripts/check_trusted_signer.ts index 1f564de292..17f6542278 100644 --- a/lazer/contracts/solana/scripts/check_trusted_signer.ts +++ b/lazer/contracts/solana/scripts/check_trusted_signer.ts @@ -40,11 +40,11 @@ async function main() { // Print storage info console.log("Storage Account Info:"); - console.log("--------------------"); + console.log("---------------------"); console.log("Top Authority:", storage.topAuthority.toBase58()); console.log("Treasury:", storage.treasury.toBase58()); - console.log("\nTrusted Signers:"); - console.log("----------------"); + console.log("\nTrusted Ed25519 Signers:"); + console.log("------------------------"); const trustedSigners = storage.trustedSigners.slice( 0, @@ -67,7 +67,7 @@ async function main() { } console.log("\nTrusted ECDSA Signers:"); - console.log("----------------"); + console.log("----------------------"); const trustedEcdsaSigners = storage.trustedEcdsaSigners.slice( 0, diff --git a/lazer/contracts/solana/scripts/verify_ed25519_message.ts b/lazer/contracts/solana/scripts/verify_ed25519_message.ts new file mode 100644 index 0000000000..ae6baa8def --- /dev/null +++ b/lazer/contracts/solana/scripts/verify_ed25519_message.ts @@ -0,0 +1,76 @@ +import * as anchor from "@coral-xyz/anchor"; +import { Program } from "@coral-xyz/anchor"; +import { PythLazerSolanaContract } from "../target/types/pyth_lazer_solana_contract"; +import * as pythLazerSolanaContractIdl from "../target/idl/pyth_lazer_solana_contract.json"; +import yargs from "yargs/yargs"; +import { readFileSync } from "fs"; +import NodeWallet from "@coral-xyz/anchor/dist/cjs/nodewallet"; +import { createEd25519Instruction } from "../src/ed25519"; +import { + sendAndConfirmTransaction, + SendTransactionError, + SYSVAR_INSTRUCTIONS_PUBKEY, + Transaction, +} from "@solana/web3.js"; + +async function main() { + let argv = await yargs(process.argv.slice(2)) + .options({ + url: { type: "string", demandOption: true }, + "keypair-path": { type: "string", demandOption: true }, + message: { type: "string", demandOption: true }, + }) + .parse(); + + const keypair = anchor.web3.Keypair.fromSecretKey( + new Uint8Array(JSON.parse(readFileSync(argv.keypairPath, "ascii"))), + ); + + const wallet = new NodeWallet(keypair); + const connection = new anchor.web3.Connection(argv.url, { + commitment: "confirmed", + }); + const provider = new anchor.AnchorProvider(connection, wallet); + + const program: Program = new Program( + pythLazerSolanaContractIdl as PythLazerSolanaContract, + provider, + ); + + const instructionMessage = Buffer.from(argv.message, "hex"); + const ed25519Instruction = createEd25519Instruction( + instructionMessage, + 1, + 12, + ); + const lazerInstruction = await program.methods + .verifyMessage(instructionMessage, 0, 0) + .accounts({ + payer: wallet.publicKey, + instructionsSysvar: SYSVAR_INSTRUCTIONS_PUBKEY, + }) + .instruction(); + + const transaction = new Transaction().add( + ed25519Instruction, + lazerInstruction, + ); + console.log("transaction:", transaction); + + try { + const signature = await sendAndConfirmTransaction( + connection, + transaction, + [wallet.payer], + { + skipPreflight: true, + }, + ); + console.log("Transaction confirmed with signature:", signature); + } catch (e) { + console.log("error", e); + console.log(e.getLogs()); + } +} + +main(); diff --git a/lazer/sdk/js/src/ed25519.ts b/lazer/contracts/solana/src/ed25519.ts similarity index 100% rename from lazer/sdk/js/src/ed25519.ts rename to lazer/contracts/solana/src/ed25519.ts diff --git a/lazer/sdk/js/package.json b/lazer/sdk/js/package.json index c2e2a2b764..6c412d5281 100644 --- a/lazer/sdk/js/package.json +++ b/lazer/sdk/js/package.json @@ -1,6 +1,6 @@ { "name": "@pythnetwork/pyth-lazer-sdk", - "version": "1.0.0", + "version": "2.0.0", "description": "Pyth Lazer SDK", "publishConfig": { "access": "public" @@ -59,8 +59,6 @@ "license": "Apache-2.0", "dependencies": { "@isaacs/ttlcache": "^1.4.1", - "@solana/buffer-layout": "^4.0.1", - "@solana/web3.js": "^1.98.0", "isomorphic-ws": "^5.0.0", "ts-log": "^2.2.7", "ws": "^8.18.0" diff --git a/lazer/sdk/js/src/constants.ts b/lazer/sdk/js/src/constants.ts index e08689283f..dadc65c431 100644 --- a/lazer/sdk/js/src/constants.ts +++ b/lazer/sdk/js/src/constants.ts @@ -1,8 +1,4 @@ -import { PublicKey } from "@solana/web3.js"; - -export const SOLANA_LAZER_PROGRAM_ID = new PublicKey( - "pytd2yyk641x7ak7mkaasSJVXh6YYZnC7wTmtgAyxPt", -); -export const SOLANA_STORAGE_ID = new PublicKey( - "3rdJbqfnagQ4yx9HXJViD4zc4xpiSqmFsKpPuSCQVyQL", -); +export const SOLANA_LAZER_PROGRAM_ID = + "pytd2yyk641x7ak7mkaasSJVXh6YYZnC7wTmtgAyxPt"; +export const SOLANA_LAZER_STORAGE_ID = + "3rdJbqfnagQ4yx9HXJViD4zc4xpiSqmFsKpPuSCQVyQL"; diff --git a/lazer/sdk/js/src/index.ts b/lazer/sdk/js/src/index.ts index 15504200d0..18e0eedf74 100644 --- a/lazer/sdk/js/src/index.ts +++ b/lazer/sdk/js/src/index.ts @@ -1,4 +1,3 @@ export * from "./client.js"; export * from "./protocol.js"; -export * from "./ed25519.js"; export * from "./constants.js"; diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 1e5aca631e..ca79527566 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -1897,7 +1897,16 @@ importers: dependencies: '@coral-xyz/anchor': specifier: ^0.30.1 - version: 0.30.1(encoding@0.1.13) + version: 0.30.1(bufferutil@4.0.9)(encoding@0.1.13)(utf-8-validate@5.0.10) + '@pythnetwork/pyth-lazer-sdk': + specifier: workspace:* + version: link:../../sdk/js + '@solana/buffer-layout': + specifier: ^4.0.1 + version: 4.0.1 + '@solana/web3.js': + specifier: ^1.98.0 + version: 1.98.0(bufferutil@4.0.9)(encoding@0.1.13)(utf-8-validate@5.0.10) devDependencies: '@types/bn.js': specifier: ^5.1.0 @@ -1938,21 +1947,15 @@ importers: '@isaacs/ttlcache': specifier: ^1.4.1 version: 1.4.1 - '@solana/buffer-layout': - specifier: ^4.0.1 - version: 4.0.1 - '@solana/web3.js': - specifier: ^1.98.0 - version: 1.98.0(bufferutil@4.0.9)(encoding@0.1.13)(utf-8-validate@5.0.10) isomorphic-ws: specifier: ^5.0.0 - version: 5.0.0(ws@8.18.1(bufferutil@4.0.9)(utf-8-validate@5.0.10)) + version: 5.0.0(ws@8.18.1(bufferutil@4.0.9)(utf-8-validate@6.0.3)) ts-log: specifier: ^2.2.7 version: 2.2.7 ws: specifier: ^8.18.0 - version: 8.18.1(bufferutil@4.0.9)(utf-8-validate@5.0.10) + version: 8.18.1(bufferutil@4.0.9)(utf-8-validate@6.0.3) devDependencies: '@cprussin/eslint-config': specifier: 'catalog:' @@ -25502,28 +25505,6 @@ snapshots: - encoding - utf-8-validate - '@coral-xyz/anchor@0.30.1(encoding@0.1.13)': - dependencies: - '@coral-xyz/anchor-errors': 0.30.1 - '@coral-xyz/borsh': 0.30.1(@solana/web3.js@1.98.0(encoding@0.1.13)) - '@noble/hashes': 1.7.1 - '@solana/web3.js': 1.98.0(bufferutil@4.0.7)(encoding@0.1.13)(utf-8-validate@6.0.3) - bn.js: 5.2.1 - bs58: 4.0.1 - buffer-layout: 1.2.2 - camelcase: 6.3.0 - cross-fetch: 3.2.0(encoding@0.1.13) - crypto-hash: 1.3.0 - eventemitter3: 4.0.7 - pako: 2.1.0 - snake-case: 3.0.4 - superstruct: 0.15.5 - toml: 3.0.0 - transitivePeerDependencies: - - bufferutil - - encoding - - utf-8-validate - '@coral-xyz/borsh@0.2.6(@solana/web3.js@1.98.0(bufferutil@4.0.7)(encoding@0.1.13)(utf-8-validate@6.0.3))': dependencies: '@solana/web3.js': 1.98.0(bufferutil@4.0.7)(encoding@0.1.13)(utf-8-validate@6.0.3) @@ -25572,12 +25553,6 @@ snapshots: bn.js: 5.2.1 buffer-layout: 1.2.2 - '@coral-xyz/borsh@0.30.1(@solana/web3.js@1.98.0(encoding@0.1.13))': - dependencies: - '@solana/web3.js': 1.98.0(bufferutil@4.0.7)(encoding@0.1.13)(utf-8-validate@6.0.3) - bn.js: 5.2.1 - buffer-layout: 1.2.2 - '@cosmjs/amino@0.30.1': dependencies: '@cosmjs/crypto': 0.30.1 @@ -45229,6 +45204,10 @@ snapshots: dependencies: ws: 8.18.1(bufferutil@4.0.9)(utf-8-validate@5.0.10) + isomorphic-ws@5.0.0(ws@8.18.1(bufferutil@4.0.9)(utf-8-validate@6.0.3)): + dependencies: + ws: 8.18.1(bufferutil@4.0.9)(utf-8-validate@6.0.3) + isows@1.0.6(ws@8.18.0(bufferutil@4.0.9)(utf-8-validate@5.0.10)): dependencies: ws: 8.18.0(bufferutil@4.0.9)(utf-8-validate@5.0.10)