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(evm): add query for confirmation height #1243

Merged
merged 9 commits into from
Feb 1, 2022

Conversation

jack0son
Copy link
Contributor

@jack0son jack0son commented Jan 21, 2022

Description

Adds a query to get the confirmation height for a given chain to allow the microservices' deposit confirmation process to validate deposits.

Once #1240 is included in the next upgrade, this information can be read from the vote result instead.

Todos

  • Unit tests
  • Manual tests
  • Documentation
  • Connect epics/issues
  • Tag type of change

Steps to Test

Expected Behaviour

Other Notes

@fish-sammy
Copy link
Contributor

Can't this be done already by querying axelard q params subspace?

@jack0son
Copy link
Contributor Author

Can't this be done already by querying axelard q params subspace?

What would the exact query for the ethereum confirmation height look like?

Can we include this PR for now so the microservice calling this can use its existing EVM module client?

Copy link
Contributor

@jcs47 jcs47 left a comment

Choose a reason for hiding this comment

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

Please don't use the legacy querier. Move this to the newly created grpc query service.

@jcs47
Copy link
Contributor

jcs47 commented Jan 31, 2022

What would the exact query for the ethereum confirmation height look like?

with grpcurl:

joaosousa@MBP-de-Joao axelarate % grpcurl -plaintext -d '{"subspace":"evm_ethereum", "key":"confirmationHeight"}' localhost:9090 cosmos.params.v1beta1.Query/Params 
{
  "param": {
    "subspace": "evm_ethereum",
    "key": "confirmationHeight",
    "value": "\"1\""
  }
}

AFAIK, grpcurl is also available as a go library. Is it too much of a hassle to incorporate it into the ms and use that instead to retrieve the confirmation height? Also, bear in mind that the "subspace" parameter must be all lower case for the query to work.

@jcs47
Copy link
Contributor

jcs47 commented Jan 31, 2022

Can't this be done already by querying axelard q params subspace?

We can fetch the same piece of information using that query instead. But I think there might be an advantage on adding this query to the evm module, in the sense that nobody would need to dig into the code and figure out the names that are used for the subspace and key -- specially if it is a piece of information that is gonna get retrieved frequently, which I think it will be (not just by our own micro-service, but other hypothetical third-party applications).

@jack0son
Copy link
Contributor Author

Can't this be done already by querying axelard q params subspace?

We can fetch the same piece of information using that query instead. But I think there might be an advantage on adding this query to the evm module, in the sense that nobody would need to dig into the code and figure out the names that are used for the subspace and key -- specially if it is a piece of information that is gonna get retrieved frequently, which I think it will be (not just by our own micro-service, but other hypothetical third-party applications).

Second this. The added query is more robust because it requires less knowledge on the client side.

@jcs47 jcs47 added the enhancement New feature or request label Feb 1, 2022
@jcs47 jcs47 merged commit 977cd09 into main Feb 1, 2022
@jcs47 jcs47 deleted the feat/evm-query-confirmation-height branch February 1, 2022 17:39
jack0son added a commit that referenced this pull request Feb 1, 2022
Co-authored-by: jcs47 <joao@axelar.network>
jack0son added a commit that referenced this pull request Feb 1, 2022
Co-authored-by: jcs47 <joao@axelar.network>

Co-authored-by: jcs47 <joao@axelar.network>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants