-
Notifications
You must be signed in to change notification settings - Fork 153
arbitrum, common, eth, internal: Implement SendExpressLaneTransactionSync #588
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
base: master
Are you sure you want to change the base?
Conversation
bac2dd7 to
633936f
Compare
ganeshvanahalli
left a comment
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.
Good stuff!
arbitrum/apibackend.go
Outdated
| } | ||
|
|
||
| func (a *APIBackend) GetAPIs(filterSystem *filters.FilterSystem) []rpc.API { | ||
| apis := ethapi.GetAPIs(a) |
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.
rest assured - I feel really guilty for the fuss, but I am afraid that the current solution is incorrect - the removal of TransactionAPI from GetAPIs() is a backwards incompatible change that potentially affects places like this one
however, I think that if you could:
- keep the new function
GetTransactionAPI() - use it inside
GetAPIs()(to preserve the old behavior) - do not manually append transaction API to the
apison the caller side
we would have a nice result
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.
We only call GetAPIs in one place, I removed the usage in backend.go
- keep the new function
GetTransactionAPI()- use it inside
GetAPIs()(to preserve the old behavior)- do not manually append transaction API to the
apison the caller sidewe would have a nice result
So you want me to revert 94cbe8d ? but keep the GetTransactionAPI() function?
I am removing GetTransactionAPI() then because then it is used in one place in the code, and hence doesn't make sense to have in its own function
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.
@pmikolajczyk41 I reverted my last, does my PR look good now?
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.
We discussed a few solutions and their pros/cons on slack. I am going to update GetApi's to return a struct which have a .toArray() function
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 feel this is a lot and no backwards compatibility was being broken with the simple solutions previously. I dont know, no opinion
94cbe8d to
01bc368
Compare
|
@pmikolajczyk41 @ganeshvanahalli ready for another look, let me know what you think d853327 Me and @pmikolajczyk41 discussed 3 different solutions #588 (comment) for context, anyways let me know |
pmikolajczyk41
left a comment
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.
thanks 🙇 LGTM
| backend.stack.RegisterAPIs(apis.Slice()) | ||
|
|
||
| service := apis.EthAPIs.TransactionAPI.Service | ||
| receiptFetcher, ok := service.(ReceiptFetcher) | ||
| if !ok { | ||
| return nil, nil, fmt.Errorf("TransactionAPI.Service does not implement ReceiptFetcher") | ||
| } |
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.
One strategy to make it easier to maintain this fork is to minimize changes to code introduced by upstream geth.
This simplifies the process of pulling changes from upstream geth to this fork, by minimizing conflicts.
Here you can do something like this:
var receiptFetcher ReceiptFetcher
for _, api := range apis {
var ok bool
receiptFetcher, ok = api.Service.(ReceiptFetcher)
if ok {
break
}
}
if receiptFetcher == nil {
// return error
}
In this way you avoid changing internal/ethapi/[backend.go, which is mostly untouched by this fork.
| signer, | ||
| txs[i], | ||
| int(rs[i].TransactionIndex), | ||
| uint64(rs[i].TransactionIndex), |
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.
Upstream geth uses int instead of uint64.
Unless this is a bug, to minimize conflicts when pulling changes from upstream geth, MarshalReceipt should remain using int here.
If you feel it is useful then you can open a PR on upstream geth with this change though 🙂
Resolves https://linear.app/offchain-labs/issue/NIT-4118/sendexpresslanetransactionsync
Pulled by OffchainLabs/nitro#4074
Questions I have
*TransactionAPIas a returned value ofGetAPIs(apiBackend Backend)the right move?, I think it is the less error prone and simpler then trying to iterate over the api's, but maybe there is a better way to do this.Normally I would change the functions name, since the returned result is different, but I don't want to due to it being in the geth submodule.