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

feat: enhance rosetta support #11609

Merged
merged 40 commits into from
May 2, 2022
Merged

feat: enhance rosetta support #11609

merged 40 commits into from
May 2, 2022

Conversation

Vritra4
Copy link
Contributor

@Vritra4 Vritra4 commented Apr 12, 2022

feat/fix to enhance rosetta support

Description

followup of #10766, #11590
Closes: #10618

we did integration test with coinbase so this PR includes all of our fixes and features requested from coinbase.

features:

  • add fee_suggetion feature for /construction/metadata
    • adding suggested_fee in result
    • added realted configurations
  • enable offline mode
    • offline-moded cosmo-sdk will return error for sequence-related APIs like /construction/metadata
  • bump rosetta-sdk-go to v0.7.7

fixes:

  • fix bugs: /block returns with nil pointer address when a request has both of index and hash
  • increase timeout to prevent timeout when genesis is huge
  • force set events about fees to success
    • to pass reconcilition, fee have to be presented as success.
    • it is available because failed transaction have no events other than fee

changelog and suggestions from #11590 applied.

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

Geoff Lee and others added 30 commits December 14, 2021 15:41
fee suggestion for construction/metadata
96a7311
rename module
ea433e7
change default gas_limit for fee suggestion
547bc58
add rosetta items in template
378308e
type fix for default-suggest-denom
5c0aea6
add default values
60c5dba
add suggestion-related config
2377100
force set to success for balance operations
262ea83
enable offline mode
0e296fb
Revert "rename module"
8fc4cc0 
This reverts commit ea433e7.
@yun-yeo
Merge branch 'master' into feature/support-rosetta
e2f9e28
increase defaultNodeTimeout for rosetta for huge genesis
503be9c
use DefaultGasLimit as DefaultSuggestGas
850a035
use default gas limit as default gas suggest for rosetta
3545ea0
add enable-default-fee-suggest for rosetta
4e76726
update config template
642a1cc
prevent bad gateway due to huge genesis
822d390
Merge branch 'master' into feature/support-rosetta
feac6af
@Vritra4 @alexanderbez @anilcse
Apply suggestions from code review
be3f786 
Co-authored-by: Aleksandr Bezobchuk <[email protected]>
Co-authored-by: Anil Kumar Kammari <[email protected]>
add price flag for rosetta standalone
3c9145e
fix rosetta data/block when block identifier is unset
9bbe992
add checking mismatched index/hash
6f1cae0
bump rosetta-sdk-go to v0.7.7
45c24d5
bump dependency
ae0c32c
add changelog
d691b88
Apply suggestions from code review
71edcbb
Apply suggestions from code review
393cf04
make gas_prices and gas_limit optional
bbc3dc0
Apply suggestions from code review
0434b4f
add changelog
1a98625
@Vritra4
Copy link
Contributor Author

Vritra4 commented Apr 12, 2022

@marbar3778 sorry to ping you. could you please review again?

alexanderbez
alexanderbez approved these changes Apr 12, 2022
View reviewed changes
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

LGTM. I don't know why we need both EnableDefaultSuggestedFee and DefaultSuggestDenom as we can get the denom from the former?

Would be nice to have someone familiar with rosetta take a look at this too. Maybe @fdymylja.

@Vritra4
Copy link
Contributor Author

Vritra4 commented Apr 12, 2022 •
edited
Loading

LGTM. I don't know why we need both EnableDefaultSuggestedFee and DefaultSuggestDenom as we can get the denom from the former?

Would be nice to have someone familiar with rosetta take a look at this too. Maybe @fdymylja.

@fdymylja
EnableDefaultSuggestedFee is about construction/metadata. coinbase wants suggested-fee in its response when the requests have no gas_limit and gas_price.
In that case, we have to choose denom in minimum-gas-prices to calculate suggested fee. That's why i added DefaultSuggestDenom.

@amaury1093 amaury1093 self-assigned this Apr 14, 2022
@Vritra4
Copy link
Contributor Author

Vritra4 commented Apr 23, 2022

variables and configs for fee suggestion have been renamed.

tac0turtle
tac0turtle approved these changes Apr 26, 2022
View reviewed changes
Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

utACK

@alexanderbez
Copy link
Contributor

This PR looks good @Vritra4. Would you mind resolving conflicts so we can get this merged?

@Vritra4 Vritra4 requested a review from a team as a code owner May 2, 2022 04:30
@codecov
Copy link

codecov bot commented May 2, 2022 •
edited
Loading

Codecov Report

Merging #11609 (caa8f59) into main (53003e1) will decrease coverage by 0.05%.
The diff coverage is 10.58%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #11609      +/-   ##
==========================================
- Coverage   66.04%   65.99%   -0.06%     
==========================================
  Files         669      669              
  Lines       70618    70681      +63     
==========================================
+ Hits        46643    46645       +2     
- Misses      21305    21367      +62     
+ Partials     2670     2669       -1
Impacted Files Coverage Δ
server/config/toml.go 66.66% <ø> (ø)
server/rosetta/client_online.go 1.35% <0.00%> (-0.09%) ⬇️
server/rosetta/config.go 0.00% <0.00%> (ø)
server/rosetta/converter.go 56.55% <0.00%> (ø)
server/config/config.go 39.28% <50.00%> (+0.47%) ⬆️
x/group/keeper/keeper.go 53.36% <0.00%> (-0.43%) ⬇️

@tac0turtle tac0turtle merged commit 38a1132 into cosmos:main May 2, 2022
@alexanderbez alexanderbez mentioned this pull request May 2, 2022
Closed
@haiyizxx
Copy link
Contributor

hey @alexanderbez @tac0turtle, could backport this to release/v0.45.x? We are on v0.45 and need the fix to pass reconciliation. Thanks!

@alexanderbez
Copy link
Contributor

I took a look over the PR and I don't see why not. We also don't really consider Rosetta under any SDK semver guarantees (I really think it should be in its own repo).

So we can backport this next week 👍

@haiyizxx
Copy link
Contributor

I took a look over the PR and I don't see why not. We also don't really consider Rosetta under any SDK semver guarantees (I really think it should be in its own repo).

So we can backport this next week 👍

Great thx! yes, put it in a separate repo totally makes sense. btw, #11857 seems related when backport

haiyizxx pushed a commit to axelarnetwork/cosmos-sdk that referenced this pull request Oct 18, 2022
@yun-yeo @alexanderbez
@anilcse @haiyizxx
feat: enhance rosetta support (cosmos#11609)
84b5f41 
* fee suggestion for construction/metadata

* rename module

* change default gas_limit for fee suggestion

* add rosetta items in template

* type fix for default-suggest-denom

* add default values

* add suggestion-related config

* force set to success for balance operations

* enable offline mode

* Revert "rename module"

This reverts commit ea433e7.

* increase defaultNodeTimeout for rosetta for huge genesis

* use DefaultGasLimit as DefaultSuggestGas

* use default gas limit as default gas suggest for rosetta

* add enable-default-fee-suggest for rosetta

* update config template

* prevent bad gateway due to huge genesis

* Apply suggestions from code review

Co-authored-by: Aleksandr Bezobchuk <[email protected]>
Co-authored-by: Anil Kumar Kammari <[email protected]>

* add price flag for rosetta standalone

* fix rosetta data/block when block identifier is unset

* add checking mismatched index/hash

* bump rosetta-sdk-go to v0.7.7

* bump dependency

* add changelog

* Apply suggestions from code review

* Apply suggestions from code review

* make gas_prices and gas_limit optional

* Apply suggestions from code review

* add changelog

* revive overwritten by cherrypick

* revive overwritten by cherry-pick

* rename local variable

* rename variables and configs

* Apply suggestions from code review

Co-authored-by: Aleksandr Bezobchuk <[email protected]>

* break validation into two if cases

* fix config toml template

Co-authored-by: yys <[email protected]>
Co-authored-by: Aleksandr Bezobchuk <[email protected]>
Co-authored-by: Anil Kumar Kammari <[email protected]>
haiyizxx pushed a commit to axelarnetwork/cosmos-sdk that referenced this pull request Feb 17, 2023
@yun-yeo @alexanderbez
@anilcse @haiyizxx
feat: enhance rosetta support (cosmos#11609)
070ed1f 
* fee suggestion for construction/metadata

* rename module

* change default gas_limit for fee suggestion

* add rosetta items in template

* type fix for default-suggest-denom

* add default values

* add suggestion-related config

* force set to success for balance operations

* enable offline mode

* Revert "rename module"

This reverts commit ea433e7.

* increase defaultNodeTimeout for rosetta for huge genesis

* use DefaultGasLimit as DefaultSuggestGas

* use default gas limit as default gas suggest for rosetta

* add enable-default-fee-suggest for rosetta

* update config template

* prevent bad gateway due to huge genesis

* Apply suggestions from code review

Co-authored-by: Aleksandr Bezobchuk <[email protected]>
Co-authored-by: Anil Kumar Kammari <[email protected]>

* add price flag for rosetta standalone

* fix rosetta data/block when block identifier is unset

* add checking mismatched index/hash

* bump rosetta-sdk-go to v0.7.7

* bump dependency

* add changelog

* Apply suggestions from code review

* Apply suggestions from code review

* make gas_prices and gas_limit optional

* Apply suggestions from code review

* add changelog

* revive overwritten by cherrypick

* revive overwritten by cherry-pick

* rename local variable

* rename variables and configs

* Apply suggestions from code review

Co-authored-by: Aleksandr Bezobchuk <[email protected]>

* break validation into two if cases

* fix config toml template

Co-authored-by: yys <[email protected]>
Co-authored-by: Aleksandr Bezobchuk <[email protected]>
Co-authored-by: Anil Kumar Kammari <[email protected]>
haiyizxx pushed a commit to axelarnetwork/cosmos-sdk that referenced this pull request Jun 5, 2023
@yun-yeo @alexanderbez
@anilcse @haiyizxx
feat: enhance rosetta support (cosmos#11609)
8e49b8e 
* fee suggestion for construction/metadata

* rename module

* change default gas_limit for fee suggestion

* add rosetta items in template

* type fix for default-suggest-denom

* add default values

* add suggestion-related config

* force set to success for balance operations

* enable offline mode

* Revert "rename module"

This reverts commit ea433e7.

* increase defaultNodeTimeout for rosetta for huge genesis

* use DefaultGasLimit as DefaultSuggestGas

* use default gas limit as default gas suggest for rosetta

* add enable-default-fee-suggest for rosetta

* update config template

* prevent bad gateway due to huge genesis

* Apply suggestions from code review

Co-authored-by: Aleksandr Bezobchuk <[email protected]>
Co-authored-by: Anil Kumar Kammari <[email protected]>

* add price flag for rosetta standalone

* fix rosetta data/block when block identifier is unset

* add checking mismatched index/hash

* bump rosetta-sdk-go to v0.7.7

* bump dependency

* add changelog

* Apply suggestions from code review

* Apply suggestions from code review

* make gas_prices and gas_limit optional

* Apply suggestions from code review

* add changelog

* revive overwritten by cherrypick

* revive overwritten by cherry-pick

* rename local variable

* rename variables and configs

* Apply suggestions from code review

Co-authored-by: Aleksandr Bezobchuk <[email protected]>

* break validation into two if cases

* fix config toml template

Co-authored-by: yys <[email protected]>
Co-authored-by: Aleksandr Bezobchuk <[email protected]>
Co-authored-by: Anil Kumar Kammari <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexanderbez alexanderbez alexanderbez approved these changes

@tac0turtle tac0turtle tac0turtle approved these changes

@aaronc aaronc Awaiting requested review from aaronc

Assignees

@amaury1093 amaury1093

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Events for failed transactions
6 participants
@Vritra4 @alexanderbez @haiyizxx @tac0turtle @amaury1093 @yun-yeo