Feat/675 on chain ouis devaddrs by bryzettler · Pull Request #431 · helium/helium-wallet-rs

bryzettler · 2024-12-06T20:04:02Z

No description provided.

The Data Credits Key is not SubDao specific.

The solana_sdk provides a `read_keypair_file` function, but it returns a Result with a Box<dyn Error>. Rather than recreate all those functions for a wrapped Keypair, I thought to start with an easy conversion from one to the other. So you can read the keypair with the solana_sdk, then very quickly turn it into a helium-lib keypair. Bring the discussion!!!

The module is named boosting so there is hopefully less confusion with the existance of `helium_anchor_lang::hexboosting` that is autogenerated.

You can directly read a helium_lib::Keypair from a file that is a solana keypair.

…g >0.30

we can not go all the way back to 0.28 because of solana-sdk. So we go back as far as we can without causing problems for upstream things like oracles.

jeffgrunewald · 2024-12-10T16:05:41Z

+    min_priority_fee: u64,
+) -> Result<u64, Error> {
+    let client_url = client.as_ref().url();
+    if client_url.contains("mainnet.helius") {


would this break if running against helius testnet; would it be safe to if client_url.contains("helius")?

helius has no priority fee api on testnet

jeffgrunewald · 2024-12-10T16:12:13Z

+        // Only take the most recent 20 maximum fees:
+        max_per_slot.sort_by(|a, b| a.0.cmp(&b.0).reverse());
+        let mut max_per_slot: Vec<_> = max_per_slot.into_iter().take(20).map(|x| x.1).collect();
+        max_per_slot.sort();


this need the second sort? this is b/c we sorted previously by time and then resorting the value itself here after the map?

yeah, further below we calculate the median so need them in order

jeffgrunewald · 2024-12-10T16:28:07Z

+                    .first()
+        {
+            ix.data = solana_sdk::compute_budget::ComputeBudgetInstruction::set_compute_unit_limit(
+                1900000,


what determines optimal values here? should these be consts or configurable?

This is just a hard coded value of compute units used for the simulation of the tx. The max you can have is 2000000 so we have a bit of wiggle room before the max. Actual compute units is pulled off the simulated response.

jeffgrunewald · 2024-12-10T17:02:28Z

+
+    let blockhash_actual = match blockhash {
+        Some(hash) => hash,
+        None => client.as_ref().get_latest_blockhash().await?,


when should this latest blockhash be requested with the specified commitment config and when is this less qualified response acceptable?

When simulating things you're ok with using the base condition but when you're forming transactions of instructions you intend to broadcast you should use a finalized or confirmed blockhash

jeffgrunewald · 2024-12-10T17:04:54Z

+    // Simulate the transaction to get the actual compute used
+    let simulation_result = client.as_ref().simulate_transaction(&snub_tx).await?;
+    if let Some(err) = simulation_result.value.err {
+        println!("Error: {}", err);


haven't noticed if we're using println! anywhere else in helium-lib - is there a convention for doing this in a more "generic" way in a library and then only using the println macro in the actual cli bin code?

that im not sure, we could probably remove these printlns fully as it returns a result with a error

jeffgrunewald · 2024-12-10T17:13:40Z

+            .await?
+            .ok_or_else(|| Error::account_not_found())?;
+
+        client


these unused results are just validations that a key is found on chain, yes?

Yeah, instruction cant be built/ran without those keys so just sanity checking they exist. Not sure if this is a good or bad pattern so lemme know?

jeffgrunewald · 2024-12-10T17:22:03Z

+    }
+
+    pub async fn update<C: AsRef<SolanaRpcClient>>(
+        _client: &C,


looking at these functions it looks to me like the pattern is that they all return an associated instruction that performs the action verb the function is named for? perhaps it's the inclusion of the client argument that i would expect these functions to actually perform that action verb but i don't know that i see any of them using the client argument? is it needed or part of a trait impl that expects a client in the off chance that the instruction requires a chain lookup to set some value?

I guess I just included the client incase logic in the fns needed to be changed. The client is mainly used to ensure pdas/keys on chain exist for the current instruction

jeffgrunewald · 2024-12-10T17:23:10Z

+    }
+
+    pub async fn ensure_exists<C: AsRef<SolanaRpcClient>>(
+        client: &C,


okay, i see the client is being used here but is it needed in all of these instructions to satisfy a trait or just convention?

Was just included for consistency/incase logic need to change. And the cli is passing client to all the fns it call for convenience. I have no good argument for keeping it if we dont want it

jeffgrunewald · 2024-12-10T17:25:59Z

+) -> Vec<(Vec<Instruction>, Vec<usize>)> {
+    // Change return type
+    let mut transactions = Vec::new();
+    let compute_ixs = vec![


i recall earlier seeing this function called and then checking to see if it only had these two compute instructions in it; would it make sense or help in any way here to return a result or option here and instead check to ensure that the instructions argument is not empty and return Err/None early instead of returning and the vec/tuple regardless and having to check at the call site if it's actually a meaningful set of instructions?

I copied this code from the helium v3 implementation and after looking at it I think that tx.len() == 2 check in the other code is redundant. I dont think we'd ever end up with a transaction of only compute_ixs based on the logic in pack.rs. Going to remove that tx.len() == 2 check.

madninja · 2024-12-17T20:06:02Z

+    let mut transactions = Vec::new();
+    let compute_ixs = vec![
+        ComputeBudgetInstruction::set_compute_unit_limit(200000),
+        ComputeBudgetInstruction::set_compute_unit_price(1),


no priority fee calculation based on the passed in instructions?

auto_compute_limit_and_price does further down

madninja · 2025-01-13T22:29:49Z

+    #[error("Wallet is not configured")]
+    WalletUnconfigured,
+    #[error("error: {0}")]
+    Error(String),


I'd like to understand the reason for this catch all error?

I saw that there existed a EncodeError::other and a DecodeError::other that acted as a catch all so I mirrored the logic here. Used here but maybe thats incorrect. https://github.com/helium/helium-config-service-cli/blob/7a40d774a0ffccd2fd26be906aee6e00fd39c5ee/src/clients/org.rs#L197-L200

michaeldjeffrey and others added 30 commits October 29, 2024 09:18

Move SubDao::dc_key() -> Dao::dc_key()

6060787

The Data Credits Key is not SubDao specific.

Add dc::burn_delegated function

8cae4db

Add boosting module, function for starting hex boost

d2bf60c

The module is named boosting so there is hopefully less confusion with the existance of `helium_anchor_lang::hexboosting` that is autogenerated.

merge imports

b1734cc

add Io error to encode error when attempting to write a keypair

c755093

Move ReadWrite trait to helium-lib

bd87fdd

You can directly read a helium_lib::Keypair from a file that is a solana keypair.

remove after bad merge

db7f998

update to use new dc_key location

2031f39

remove useless ? and Ok

b2f4780

move ReadWrite back to wallet. avoid file io in helium-lib

7a67eed

upgrade spl-account-compression to version compatible with anchor-lan…

e827eeb

…g >0.30

move back to anchor-lang 0.29

dc47049

update helium-anchor-gen with looser dep around anchor-client

040fc93

we can not go all the way back to 0.28 because of solana-sdk. So we go back as far as we can without causing problems for upstream things like oracles.

go back to master helium-anchor-gen that has updated anchor-lang >=0.28

0e750c9

wip moving iot_routing_manager sdk here

9ee2bb8

Pdas and instructions creators for iot_routing_manager plus a refactor

3b38d76

added update org

74c9d34

move config key to hem

381423a

Us as_ref for solana client

1e6ee06

added send instructions and restructured sol transaction utils

3f245d4

structure client better

3b7f815

add wrapper fn for getting the SolanaRpcClient

1ea9ba3

.

02d5791

added wallet/payer logic to solana_client

3abf8a9

Pass tpu helium-lib

e14d61a

add AsRef for SolanaClient

174b779

Building

3d10078

bump bubblegum

3b2249d

wip

0a36e98

bryzettler added 4 commits December 5, 2024 16:50

Fix errors after merging master

0623a99

testing

c8a587f

remove cargo patches

d4ef2c9

self review

ca0308c

bryzettler requested review from ChewingGlass, jeffgrunewald, madninja and michaeldjeffrey December 6, 2024 21:05

jeffgrunewald reviewed Dec 10, 2024

View reviewed changes

bryzettler added 2 commits December 10, 2024 15:43

remove unused priority_fee

c9431ea

pr feedback

e79b802

madninja reviewed Dec 17, 2024

View reviewed changes

bryzettler added 6 commits December 18, 2024 16:35

fold helium_sub_daos back into dao

7557c35

fold helium_entity_manger back into dao/asset

9c907a8

Use dao/asset in iot

4642b8f

update refrences of helium_entity_manger

8fde91e

PR feedback

fbdf4d3

fix

25147f3

bryzettler force-pushed the feat/675-on-chain-ouis-devaddrs branch from 0031f22 to 25147f3 Compare December 20, 2024 19:15

Merge branch 'master' into feat/675-on-chain-ouis-devaddrs

d46f7e9

madninja reviewed Jan 13, 2025

View reviewed changes

bryzettler added 2 commits January 14, 2025 13:36

fold metaplex into asset

2527773

pr feedback

0dd0e45

bryzettler force-pushed the feat/675-on-chain-ouis-devaddrs branch from 711d264 to 0dd0e45 Compare January 14, 2025 20:55

bryzettler added 4 commits January 15, 2025 09:03

remove refrences to tpu

16239b5

pr feedback tweaks

fb7c2c4

fix compute budget estimate

4d8ea0f

unpatch crates

a62acdb

bryzettler closed this Feb 20, 2025

Conversation

bryzettler commented Dec 6, 2024

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!

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!

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!

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bryzettler Jan 14, 2025 • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bryzettler Jan 14, 2025 •

edited

Loading