-
Notifications
You must be signed in to change notification settings - Fork 10
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
Add debug_flowHeight API endpoint #669
Add debug_flowHeight API endpoint #669
Conversation
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
@zhangchiqing @m-Peter What do you think of the API endpoint name? |
api/debug.go
Outdated
@@ -361,6 +361,24 @@ func (d *DebugAPI) TraceCall( | |||
return tracer.GetResult() | |||
} | |||
|
|||
// FlowHeight returns the Flow height for the given EVM block. | |||
func (d *DebugAPI) FlowHeight( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func (d *DebugAPI) FlowHeight( | |
func (d *DebugAPI) FlowHeightByEVMBlock( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we would have to query it by: eth_flowHeightByEVMBlock
, it's rather easy to get it wrong 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FlowHeightByBlock
maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we would have to query it by: eth_flowHeightByEVMBlock, it's rather easy to get it wrong 😅
Why it's easy to get wrong? Sorry, I didn't get it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it seems to me as a rather long name, with various capital letters and mixed terminology (height vs block). Personally, I wouldn't remember it easily 😅 I don't have a strong preference for the name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Is TraceBlockByNumber
auto mapped to debug_traceBlockByNumber
? If so, debug_flowHeightByEVMBlock
is the same naming convention, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, the naming convention is just to prepend the namespace (e.g. eth_
/ net
/ debug
) and lowercase the first character of the method name. debug_flowHeightByEVMBlock
is also fine, I believe we will have this command documented, so it's not like we will write it by hand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though I am not entirely sure if it will be debug_flowHeightByEVMBlock
or debug_flowHeightByEvmBlock
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though I am not entirely sure if it will be debug_flowHeightByEVMBlock or debug_flowHeightByEvmBlock.
Oh, I see your concern now. Yeah, let's find out.
I'm ok that we name it to FlowHeightByBlock
to avoid being mapped to debug_flowHeightByEvmBlock
, which looks weird. In that case, we better mention EVM block in the comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried this locally, and it will be mapped to debug_flowHeightByEVMBlock
, so only the first character of the method name gets lower-cased. The rest remains intact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome 👏
@janezpodhostnik I forgot to mention, there's also this map here: https://github.com/onflow/flow-evm-gateway/blob/main/api/api.go#L37-L39, so we might want to update it, when we settle on the endpoint's name 🙏 |
Thanks. Added. I also changed the method name as per the current consensus. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@zhangchiqing are you ok with the naming? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
dd2b134
into
feature/local-tx-reexecution
Description
Added
debug_flowHeight
API endpoint so you can get the flow height at a given EVM blockFor contributor use:
master
branchFiles changed
in the Github PR explorer