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.

Sign up for GitHub

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

Jump to bottom

GetTXFee API Refactor #3598

Closed
wants to merge 13 commits into from
Closed

GetTXFee API Refactor #3598

wants to merge 13 commits into from

Conversation

samliok
Copy link
Contributor

@samliok samliok commented Dec 16, 2024

Why this should be merged

Removes the info.GetFeeTx endpoint which is now deprecated. The api is moved to the XChainClient which only returns relevant fields now.

How this works

How this was tested

Added a test to CI.

Need to be documented in RELEASES.md?

Yes

@samliok samliok self-assigned this Dec 16, 2024
@samliok samliok marked this pull request as draft December 16, 2024 19:32
@meaghanfitzgerald meaghanfitzgerald self-requested a review December 16, 2024 20:17
@meaghanfitzgerald
Copy link
Contributor

looks good so far, would just double check that none of the wallet service functionality for the p-chain is broken by removing the endpoint. It shouldn't, as a new tx fee getter was already implemented pre etna, but it doesn't hurt to check that all those tests are passing

@samliok samliok linked an issue Dec 16, 2024 that may be closed by this pull request
GetTXFee API Refactor #3597
Open
3 tasks
@samliok samliok marked this pull request as ready for review December 16, 2024 21:59
StephenButtolph
StephenButtolph reviewed Dec 16, 2024
View reviewed changes
vms/avm/client.go Outdated Show resolved Hide resolved
vms/avm/service.md Outdated Show resolved Hide resolved
wallet/chain/p/context.go
if err != nil {
return nil, err
}

return &builder.Context{
NetworkID: networkID,
AVAXAssetID: avaxAssetID,
StaticFeeConfig: fee.StaticConfig{
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove StaticFeeConfig entirely? Perhaps we should add a TODO to the context to remove that field (as I don't think we need it anymore).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

created this, will tackle in another pr
#3601

meaghanfitzgerald
meaghanfitzgerald reviewed Dec 17, 2024
View reviewed changes
vms/avm/service.md Outdated Show resolved Hide resolved
vms/avm/service.md Outdated Show resolved Hide resolved
vms/avm/service.md Outdated Show resolved Hide resolved
meaghanfitzgerald
meaghanfitzgerald approved these changes Dec 17, 2024
View reviewed changes
Copy link
Contributor

@meaghanfitzgerald meaghanfitzgerald left a comment

Choose a reason for hiding this comment

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

lgtm, e2e pre etna is failling but i think that is expected?

@samliok
Copy link
Contributor Author

samliok commented Dec 17, 2024

lgtm, e2e pre etna is failling but i think that is expected?

yes its expected, but not sure why e2e_existing_network is failing

@meaghanfitzgerald
Copy link
Contributor

lets see if it fails after merging master

@samliok
Copy link
Contributor Author

samliok commented Dec 19, 2024

closing this pr for #3610 since a more extensive pr is needed for the e2e tests to pass.

@samliok samliok closed this Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@StephenButtolph StephenButtolph StephenButtolph left review comments

@meaghanfitzgerald meaghanfitzgerald meaghanfitzgerald approved these changes

Assignees

@samliok samliok

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

GetTXFee API Refactor
3 participants
@samliok @meaghanfitzgerald @StephenButtolph