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

Leaderboard query #29

Merged
merged 43 commits into from
Nov 24, 2019
Merged

Leaderboard query #29

merged 43 commits into from
Nov 24, 2019

Conversation

mrsmkl
Copy link

@mrsmkl mrsmkl commented Nov 22, 2019

Adds the following

  • leaderboard query
  • reading validator history
  • read contract addresses from registry
  • reading extra logs to get the epoch rewards (outdated)
  • counting attestation events

Copy link

@nambrot nambrot left a comment

Choose a reason for hiding this comment

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

Should be minor adjustments. Please in the future submit separate PRs there is a lot of seeparatee functionality in this PR

apps/ethereum_jsonrpc/lib/ethereum_jsonrpc.ex Outdated Show resolved Hide resolved
apps/ethereum_jsonrpc/lib/ethereum_jsonrpc/contract.ex Outdated Show resolved Hide resolved
apps/explorer/lib/explorer/celo/account_reader.ex Outdated Show resolved Hide resolved
(SELECT claims.claimed_address AS address, COALESCE(SUM(value),0) AS value
FROM address_current_token_balances, claims
WHERE address_hash=claims.claimed_address
AND token_contract_address_hash='\\x88f24de331525cf6cfd7455eb96a9e4d49b7f292'
Copy link

Choose a reason for hiding this comment

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

What are these constants?

Copy link
Author

Choose a reason for hiding this comment

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

It's the address for stable token. I guess this query will be thrown away after the stake off so it doesn't matter much that it's hard coded.

Copy link

Choose a reason for hiding this comment

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

Please add a comment for what this query does, or at least a link to to documentation

contract_address: token_contract_address_hash,
function_name: "balanceOf",
args: [address_hash],
block_number: block_number
block_number: block_number,
gasprice: "1000000000000000000"
Copy link

Choose a reason for hiding this comment

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

Why gasprice here?

Copy link
Author

Choose a reason for hiding this comment

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

For some reason, it complained about smaller than minimum gas price when reading from some previous blocks. Added issue here celo-org/celo-blockchain#638

Copy link

Choose a reason for hiding this comment

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

Please leave a comment here with a reference to the ticket

apps/explorer/lib/explorer/token/balance_reader.ex Outdated Show resolved Hide resolved
create(index(:celo_validator_history, [:validator_address, :block_number, :index], unique: true))
create(index(:celo_validator_history, [:block_number, :index], unique: true))

if Mix.env() != :test do
Copy link

Choose a reason for hiding this comment

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

Is this a common pattern?

Copy link
Author

Choose a reason for hiding this comment

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

Let's add an issue for removing these.

Copy link
Author

Choose a reason for hiding this comment

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

apps/indexer/lib/indexer/transform/token_transfers.ex Outdated Show resolved Hide resolved
@nambrot nambrot assigned mrsmkl and unassigned nambrot Nov 23, 2019
Update to handle new transaction fields
Reading logs that have no associated transaction
@mrsmkl mrsmkl assigned nambrot and unassigned mrsmkl Nov 24, 2019
Copy link

@nambrot nambrot left a comment

Choose a reason for hiding this comment

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

Please add a bit more documentation and address all the comments. Given the current time sensitivity, I'll approve the PR, but in the future, please consider smaller PRs

(SELECT claims.claimed_address AS address, COALESCE(SUM(value),0) AS value
FROM address_current_token_balances, claims
WHERE address_hash=claims.claimed_address
AND token_contract_address_hash='\\x88f24de331525cf6cfd7455eb96a9e4d49b7f292'
Copy link

Choose a reason for hiding this comment

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

Please add a comment for what this query does, or at least a link to to documentation

ORDER BY score
""")

# IO.inspect(result)
Copy link

Choose a reason for hiding this comment

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

Remove

contract_address: token_contract_address_hash,
function_name: "balanceOf",
args: [address_hash],
block_number: block_number
block_number: block_number,
gasprice: "1000000000000000000"
Copy link

Choose a reason for hiding this comment

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

Please leave a comment here with a reference to the ticket

@@ -112,16 +114,30 @@ defmodule Explorer.Celo.AccountReader do
end

defp fetch_account_data(account_address) do
res =
Copy link

Choose a reason for hiding this comment

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

Unnecessary res

{function_name, response}
end)

{:ok, [address]} = res["getAddressForString"]
Copy link

Choose a reason for hiding this comment

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

What happens when res does not have the key?

@@ -110,6 +111,23 @@ defmodule Indexer.Block.Fetcher do
struct!(__MODULE__, named_arguments)
end

# defp append_logs(extra_logs, %{logs: logs, receipts: receipts}) do
Copy link

Choose a reason for hiding this comment

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

Remove

%{accounts: celo_accounts, validators: celo_validators, validator_groups: celo_validator_groups} =
CeloAccounts.parse(logs),
%{token_transfers: normal_token_transfers, tokens: normal_tokens} = TokenTransfers.parse(logs),
# %{token_transfers: fee_token_transfers, tokens: fee_tokens} =
Copy link

Choose a reason for hiding this comment

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

Remove

@@ -152,6 +183,7 @@ defmodule Indexer.Block.Fetcher do
token_transfers: token_transfers,
transactions: transactions_with_receipts
}),
# addresses = Enum.filter(addresses_raw, fn a -> a.hash != "0x0000000000000000000000000000000000000000" end),
Copy link

Choose a reason for hiding this comment

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

Remove

@@ -4,7 +4,7 @@ defmodule Indexer.Fetcher.CeloAccountsTest do

import Mox

alias Explorer.Chain.{Address, Hash, CeloAccount}
# alias Explorer.Chain.{Address, Hash, CeloAccount}
Copy link

Choose a reason for hiding this comment

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

Remove

@nambrot nambrot assigned mrsmkl and unassigned nambrot Nov 24, 2019
@timmoreton timmoreton merged commit 909682b into master Nov 24, 2019
@mrsmkl mrsmkl mentioned this pull request Nov 25, 2019
diminator added a commit that referenced this pull request Nov 25, 2019
@carterqw2 carterqw2 deleted the mrsmkl/leaderboard-1 branch February 6, 2023 13:31
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.

3 participants