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

feat: add block proposer to addresses_with_balance_change #203

Merged
merged 1 commit into from
Dec 16, 2024

Conversation

joel-u410
Copy link
Contributor

Per this comment on #199, block proposers also need balances fetched.

Figured this was an easy change -- @Fraccaman here's a PR implementing it, assuming this is the right fix here.

@joel-u410
Copy link
Contributor Author

joel-u410 commented Dec 12, 2024

Hmm, actually not sure this is right -- I'm seeing proposer_address like cafad8da813bae48779a4219a74632d5dca49737 (which I think is the consensus key / tendermint address) but it probably needs to be e.g. tnam1qyctcwkgthr06k7lx38zmjka5dakmvhhyyr0zafu (the namada validator address).

Is there an easy way to get to the tnam address from the tendermint key/addr? It looks like the become_validator transactions have both an address and consensus_key field, but that doesn't cover the genesis validators, and the key is in a different format (but I'm sure could be converted).

Update: I see namadac can do it via the find-validator command. I'll take a look at what it's doing and see if it can be replicated in namada-indexer. Ultimately, it would be great to store both of these addresses in the validators table for every validator.

@joel-u410 joel-u410 changed the title fix: add block proposer to addresses_with_balance_change feat: add block proposer to addresses_with_balance_change Dec 13, 2024
@Fraccaman
Copy link
Member

you are right! we can only query balances given a namada address. I think you can fetch the proposal then call

RPC.vp()
                    .pos()
                    .validator_by_tm_addr(context.client(), &tm_addr)
                    .await` to get the corresponding namada address

Agree it would be better to store these in a separate table but not a big deal for now.

@joel-u410
Copy link
Contributor Author

Should this PR target 1.0.0-maint?

@Fraccaman
Copy link
Member

yep! thanks

@joel-u410 joel-u410 force-pushed the joel/block-proposer-balances branch from cbf4f14 to 04d0f7f Compare December 13, 2024 16:23
@joel-u410 joel-u410 marked this pull request as draft December 13, 2024 16:39
@joel-u410
Copy link
Contributor Author

This compiles, but doesn't quite seem to be working yet (I'm getting None for the validator address) -- will mark ready when I've confirmed it's working.

@joel-u410
Copy link
Contributor Author

Ah, I think I've solved it -- turns out the RPC is case sensitive, and tm_addr must be uppercase (but is lowercase in shared::block::Block!)

@joel-u410 joel-u410 force-pushed the joel/block-proposer-balances branch 2 times, most recently from e8a1fa1 to ecf924d Compare December 13, 2024 16:51
@joel-u410 joel-u410 changed the base branch from main to 1.0.0-maint December 13, 2024 16:52
@joel-u410 joel-u410 marked this pull request as ready for review December 13, 2024 16:52
@joel-u410 joel-u410 force-pushed the joel/block-proposer-balances branch from ecf924d to fb31083 Compare December 13, 2024 16:56
Comment on lines +178 to +182
let all_balance_changed_addresses = addresses
.iter()
.chain(block_proposer_address.iter())
.cloned()
.collect::<HashSet<_>>();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

After rebasing to 1.0.0-maint, this is going to have merge conflicts with main -- though it should be relatively straightforward to resolve. I kept the pattern of combining HashSets into all_balance_changed_addresses here, in anticipation of merging to main where it also has pgf_receipient_addresses. One thing to watch out for is that here I used .iter().chain() instead of .union() which will make it simpler to combine the 3 HashSets when this is merged to main. So, when merging, you should keep this .iter().chain() version and simply add the line

        .chain(pgf_receipient_addresses.iter())

@Fraccaman Fraccaman added this pull request to the merge queue Dec 16, 2024
Merged via the queue into anoma:1.0.0-maint with commit 7cf71cf Dec 16, 2024
8 checks passed
@joel-u410 joel-u410 deleted the joel/block-proposer-balances branch December 16, 2024 18:02
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