-
Notifications
You must be signed in to change notification settings - Fork 189
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
Added eth namespace Block Header apis #1088
Conversation
Added Engine() to backend * We need it when calculating proposer of the block. * Regenerated backend_mock.go also. Add rpcMarshalHeader * This is for supporting Etheruem compatible format. Add Methods for BlockNonce * Added implementations from Ethereum. Make BaseFee as protocol params and make extradata for eth namespace. * Value of BaseFee and Extradata are considered by klaytn developers. * While defining BaseFee as protocol params, the parts that previously defined baseFee were also modified.
Added Engine() to backend * We need it when calculating proposer of the block.
ExtraData should be empty which is discussed by Klaytn devs. GasLimit: current ethereum gas limit x 30
GetHeaderByNumber in Ethereum always return nil because the backend returns nil at error position.
api/api_ethereum.go
Outdated
klaytnHeader, err := api.publicBlockChainAPI.GetHeaderByNumber(ctx, number) | ||
// In Ethereum, err is always nil because the backend of Ethereum always return nil. | ||
err = nil |
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.
It can be looked weird, but currently Ethereum backend always return nil at error position.
Klaytn devs decided to serve this field as empty when serving eth namespace api.
Co-authored-by: yumiel yoomee1313 <yumiel.ko@krustuniverse.com>
Co-authored-by: yumiel yoomee1313 <yumiel.ko@krustuniverse.com>
results := reflect.ValueOf(&api).MethodByName(testAPIName).Call( | ||
[]reflect.Value{ | ||
reflect.ValueOf(context.Background()), | ||
reflect.ValueOf(blockParam), | ||
}, | ||
) |
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.
Reflect package is used due to call multiple api tests at a time
Inserting error directly is a bad practice. So use another way to handle it. Use logger exactly.
6b574a4
Co-authored-by: Donghwan Aidan Kwon <48199072+aidan-kwon@users.noreply.github.com>
All block signatures should be recoverable, and miner is the field Klaytn should provide the correct value. It's not the field dummy value is allowed.
Gave values to header so that we can check whether rpcMarshalHeader assigns all fields values properly
Just compare field and values using json marshaling, it is more simpler
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.
Thank you!
Proposed changes
Added
Engine()
tobackend
interface andCNAPIBackend
also.(&EthereumAPI{}).Miner
method.backend_mock.go
also because new methodEngine
is added.Added
rpcMarshalHeader
function.Added Methods for
BlockNonce
Made
BaseFee
as protocol params for eth namespace.BaseFee
andExtradata
are considered by klaytn developers.BaseFee
as protocol params, the parts that previously defined baseFee were also modified.Types of changes
Please put an x in the boxes related to your change.
Checklist
Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.
$ make test
)Related issues
Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...