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

[api] return evm address instead of oasis addr in RuntimeEvmBalance #492

Merged
merged 1 commit into from
Jul 31, 2023

Conversation

Andrew7234
Copy link
Collaborator

According to the openapi spec, the token_contract_address in RuntimeEvmBalance should be the EVM address and not the oasis address.

Need to confirm with explorer team that the currently returned oasis address can be replaced by the evm address. If not, we can return both the oasis and evm addresses (though the oasis address can be derived from the eth address).

@Andrew7234 Andrew7234 force-pushed the andrew7234/runtime-account-evm-balance branch from 52f134a to 70d3234 Compare July 20, 2023 03:15
@csillag
Copy link
Contributor

csillag commented Jul 20, 2023

Need to confirm with explorer team that the currently returned oasis address can be replaced by the evm address. I

If we do that, it's a breaking change, which would need synchronized Nexus and Explorer releases to avoid breakage.

I think adding token_contract_addr_eth besides the unchanged token_contract_addr would provide a much smoother upgrade path.

Also, we have a switch on Explorer UI (well, at least for some parts) that supports choosing displaying Oasis and ETH addresses, according to the user's preferences. So having the Oasis address ready is definitely a plus.

@csillag
Copy link
Contributor

csillag commented Jul 20, 2023

Thanks, I like this version much better!

@csillag
Copy link
Contributor

csillag commented Jul 20, 2023

This will be used by oasisprotocol/explorer#762.

@Andrew7234 Andrew7234 force-pushed the andrew7234/runtime-account-evm-balance branch 3 times, most recently from 845d216 to 595c70c Compare July 21, 2023 00:58
Copy link
Contributor

@mitjat mitjat left a comment

Choose a reason for hiding this comment

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

Thank you for this patch, and all the others!

api/spec/v1.yaml Outdated Show resolved Hide resolved
@Andrew7234 Andrew7234 force-pushed the andrew7234/runtime-account-evm-balance branch from 595c70c to 57d2972 Compare July 25, 2023 19:16
@Andrew7234 Andrew7234 force-pushed the andrew7234/erc-event-info branch from 1ff636c to 1c2e9a3 Compare July 27, 2023 15:11
@Andrew7234 Andrew7234 force-pushed the andrew7234/runtime-account-evm-balance branch from 57d2972 to 9f43769 Compare July 27, 2023 15:27
@Andrew7234 Andrew7234 force-pushed the andrew7234/erc-event-info branch from 1c2e9a3 to b661cdc Compare July 31, 2023 14:41
Base automatically changed from andrew7234/erc-event-info to main July 31, 2023 14:53
[api] add runtimeEvmBalance.token_contract_addr_eth instead of replacing

nit
@Andrew7234 Andrew7234 force-pushed the andrew7234/runtime-account-evm-balance branch from 9f43769 to f578b38 Compare July 31, 2023 15:16
@Andrew7234 Andrew7234 enabled auto-merge July 31, 2023 15:20
@Andrew7234 Andrew7234 merged commit 406e905 into main Jul 31, 2023
@Andrew7234 Andrew7234 deleted the andrew7234/runtime-account-evm-balance branch July 31, 2023 15:21
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.

4 participants