-
Notifications
You must be signed in to change notification settings - Fork 66
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
Enrich contract events to contain data needed by child-chain and watcher #686
Conversation
c092551
to
6dc88a2
Compare
plasma_framework/package.json
Outdated
"@codechecks/client": "^0.1.10", | ||
"@openzeppelin/test-helpers": "^0.5.6", |
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.
The older version of the test helpers package could be uninstalled
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, I decided to stay with the old version and remove the new one. I noticed I didn't use the new version.
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.
@pgebal Can we do a clean uninstall then? looks like it's still there in package-lock
f31f72b
to
901fbf3
Compare
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.
@pgebal the tests do not pass I would like to see a gas report and compare the impact
@kevsul please take a look at this commit: f31f72b I deleted that check because web3py can't handle complex events (I guess because of Removing this check does not invalidate the python test. |
I think that's fine. |
uint256 utxoPos, | ||
bytes rlpOutputTx |
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.
hmmm, do we need rlpOutputTx
? I don't think we do. I thin it's only utxoPos
that we pull out of calldata.
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.
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.
rlpOutputTx
gets renamed into:
{"outputTxInclusionProof", :output_tx_inclusion_proof},
{"rlpOutputTx", :output_tx},
{"utxoPos", :utxo_pos}
so it's output_tx
.
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 need it in our current code @InoMurko . Check fields.ex
and exit processor core. We are currently getting it from call data. We could work around that by getting the tx from rocksdb storage.
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.
yeah fields.ex
is a mapping module for structural decoding of ABI data, it's not really used anywhere.
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.
@InoMurko @boolafish how about here: https://github.com/omgnetwork/elixir-omg/blob/a7b4188c938390b13d069f40d941507ca79faca3/apps/omg_watcher/lib/omg_watcher/exit_processor/exit_info.ex#L66
Isn't it the exiting tx bytes from call data?
Called here: https://github.com/omgnetwork/elixir-omg/blob/a7b4188c938390b13d069f40d941507ca79faca3/apps/omg_watcher/lib/omg_watcher/exit_processor/core.ex#L188
… InFlightExitStarted event
398dcce
to
ae28352
Compare
During a call with @thec00n it was decided to remove: |
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 discussed this with @pgebal and we covered my concerns in terms of the increased gas usage. In general I think we should plan for a targeted feature team working on reducing gas costs before v2 goes live.
Update data stored in events so it contains everything needed by child-chain and watcher, so they do not have to get if from call data.
Motivation for the changes are:
Changes come with a cost of increased gas.
References #659
Helpful link:
https://omisego.atlassian.net/wiki/spaces/FT/pages/667746350/Minimal+Contract+Events+Review