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

Andrew7234/runtime tx denoms #726

Merged
merged 1 commit into from
Jul 18, 2024
Merged

Conversation

Andrew7234
Copy link
Collaborator

@Andrew7234 Andrew7234 commented Jul 11, 2024

Adds denomination fields for runtime transactions as fee_symbol and amount_symbol.

Also updates is_likely_native_token_xfer flag to be more accurate, api: is_likely_native_token_transfer should be true only for staking.Transfer with native denomination

}

// StringifyDenomination returns a string representation of the given denomination.
// This is simply the denomination's symbol; notably, for the native denomination,
// this is looked up from network config.
func (m *processor) StringifyDenomination(d sdkTypes.Denomination) string {
func StringifyDenomination(runtimeMetadata *sdkConfig.ParaTime, d sdkTypes.Denomination) string {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The alternative to this refactor would be to copy these methods to extract.go, which would leave us with duplicate processor.StringifyDenomination as well as a StringifyDenomination functions in the same package. Not sure which is cleaner/more idiomatic, open to suggestions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

this looks fine. if we ever need it outside the runtime blocks analyzer, we can move it to analyzer/util. a lot of stringifiers are there

storage/client/client.go Show resolved Hide resolved
storage/migrations/01_runtimes.up.sql Outdated Show resolved Hide resolved
}

// StringifyDenomination returns a string representation of the given denomination.
// This is simply the denomination's symbol; notably, for the native denomination,
// this is looked up from network config.
func (m *processor) StringifyDenomination(d sdkTypes.Denomination) string {
func StringifyDenomination(runtimeMetadata *sdkConfig.ParaTime, d sdkTypes.Denomination) string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this looks fine. if we ever need it outside the runtime blocks analyzer, we can move it to analyzer/util. a lot of stringifiers are there

@Andrew7234 Andrew7234 force-pushed the andrew7234/runtime-tx-denoms branch 3 times, most recently from 1e3b4ce to 15faaac Compare July 18, 2024 20:50
@Andrew7234
Copy link
Collaborator Author

Huh we still have damask emerald disabled in our e2e_regression tests. I tried again and still had errors trying to query historical balances in the runtime finalizeFastSync. The comment says we probably need to redeploy the archive node now that 5540 is merged, which seems to have happened. The archive node is on labs infra and is running this image tag, which was built 5mo ago and should have worked. Will file a follow-up ticket to investigate further

update e2e_regression test cases

rename runtimeMetadata -> sdkPT for consistency

nits

changelog
@Andrew7234 Andrew7234 force-pushed the andrew7234/runtime-tx-denoms branch from 79cc649 to 399f701 Compare July 18, 2024 21:16
@Andrew7234 Andrew7234 merged commit 25b10c5 into main Jul 18, 2024
16 checks passed
@Andrew7234 Andrew7234 deleted the andrew7234/runtime-tx-denoms branch July 18, 2024 21: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.

2 participants