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

[WIP, DONOT MERGE]: Fix FeeOps duplicated index issue and improve geth-sdk. #103

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jacquescaocb
Copy link

@jacquescaocb jacquescaocb commented Sep 29, 2023

Fixes # .
1, Fix FeeOps duplicated index Ops issue.
2, Added common function EstimateGas declaration in the type interface.
3, Fix TranceOps wrong native token issue.

Motivation

Solution

Open questions

@jacquescaocb jacquescaocb force-pushed the jac/evm-consolidation branch 2 times, most recently from fc22eb2 to 7701430 Compare September 29, 2023 19:20
// the current pending state of the backend blockchain. There is no guarantee that this is
// the true gas limit requirement as other transactions may be added or removed by miners,
// but it should provide a basis for setting a reasonable default.
EstimateGas(ctx context.Context, msg Eth.CallMsg) (uint64, error)
Copy link
Contributor

Choose a reason for hiding this comment

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

EstimateGas is the built-in function of ethclient, it shouldn't be in our client interface

@@ -130,7 +130,7 @@ func TransferOps(tx *evmClient.LoadedTransaction, startIndex int) []*RosettaType
return ops
}

func FeeOps(tx *evmClient.LoadedTransaction) []*RosettaTypes.Operation {
func FeeOps(tx *evmClient.LoadedTransaction, native_currency *RosettaTypes.Currency) []*RosettaTypes.Operation {
Copy link
Contributor

Choose a reason for hiding this comment

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

this function is used by all EVM chains, modify the shared util function here especially the function args can potentially break all downstream dependents, usually EVM chain should implement it according to its own flavour

also all EVM and L2 use ETH as native token, we shouldn't only modify the shared util function for BNB

@@ -203,6 +203,7 @@ func FeeOps(tx *evmClient.LoadedTransaction) []*RosettaTypes.Operation {
func TraceOps(
calls []*evmClient.FlatCall,
startIndex int,
native_currency *RosettaTypes.Currency,
Copy link
Contributor

Choose a reason for hiding this comment

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

this function is used by all EVM chains, modify the shared util function here especially the function args can potentially break all downstream dependents, usually EVM chain should implement it according to its own flavour

also all EVM and L2 use ETH as native token, we shouldn't only modify the shared util function for BNB

@GeekArthur
Copy link
Contributor

@jacquescaocb please hold off to do consolidation until the alignment of plan/execution is finalized

@jacquescaocb jacquescaocb changed the title Fix FeeOps duplicated index issue and improve geth-sdk. [WIP]: Fix FeeOps duplicated index issue and improve geth-sdk. Oct 3, 2023
@jacquescaocb jacquescaocb changed the title [WIP]: Fix FeeOps duplicated index issue and improve geth-sdk. [WIP, DONOT MERGE]: Fix FeeOps duplicated index issue and improve geth-sdk. Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants