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

Add erigon_getBalanceChangesInBlock RPC endpoint #4609

Conversation

tsutsu
Copy link
Contributor

@tsutsu tsutsu commented Jul 2, 2022

No description provided.

@tsutsu tsutsu marked this pull request as draft July 2, 2022 18:38
@tsutsu tsutsu marked this pull request as ready for review July 2, 2022 18:58
if err != nil {
t.Errorf("calling GetBalanceChangesInBlock resulted in an error: %v", err)
}
PrintChangedBalances(balances)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you be able to put couple of asserts here?

@mandrigin
Copy link
Collaborator

@tsutsu please also update the REAMDE for rpcdaemon and include the new API method

@tjayrush
Copy link
Contributor

tjayrush commented Jul 4, 2022

Should this be in the eth_ namespace? I feel like that will come back to haunt you at some point. Isn't the eth_ namespace reserved for things available across multiple clients and maybe even requiring an EIP? I did a quick Google search. I didn't find it elsewhere. Maybe I missed it.

@tsutsu
Copy link
Contributor Author

tsutsu commented Jul 6, 2022

Should this be in the eth_ namespace? I feel like that will come back to haunt you at some point. Isn't the eth_ namespace reserved for things available across multiple clients and maybe even requiring an EIP? I did a quick Google search. I didn't find it elsewhere. Maybe I missed it.

I agree; this isn't a standardized endpoint (and in fact wouldn't be supportable in other clients due to only being practical with a ChangeSet-like index) so shouldn't be under eth_. Wasn't sure of the correct namespace to put this under. Will change to whatever is suggested. (And will then update the README to mention.)

@mandrigin
Copy link
Collaborator

@tsutsu let's add real test asserts and move it to erigon_ for now

Copy link
Collaborator

@mandrigin mandrigin left a comment

Choose a reason for hiding this comment

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

(1) add assets to this tes
(2) move to erigon_ namespace

@fatemebagherii
Copy link
Contributor

(1) add assets to this tes (2) move to erigon_ namespace

Here are some assertions: 25be3d6
Are they sufficient?

@AskAlexSharov AskAlexSharov changed the title Add eth_getBalanceChangesInBlock RPC endpoint Add erigon_ getBalanceChangesInBlock RPC endpoint Jul 13, 2022
@AskAlexSharov AskAlexSharov changed the title Add erigon_ getBalanceChangesInBlock RPC endpoint Add erigon_getBalanceChangesInBlock RPC endpoint Jul 13, 2022
@AskAlexSharov
Copy link
Collaborator

I will add line to cmd/rpcdaemon/readme.md

@AskAlexSharov
Copy link
Collaborator

@mandrigin let's merge?

@mandrigin mandrigin merged commit b6440ee into ledgerwatch:devel Jul 15, 2022
@mandrigin
Copy link
Collaborator

Done!

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.

None yet

6 participants