feat: create ERC20OverwriteFactory utils #12
Conversation
tamaralipows
force-pushed
the
tnl/ENG-3750-erc20-overwrite-factory
branch
from
7a80cd3 to
86457cf
Compare
src/protocol/vm/utils.rs
Outdated
| let code = format!( | ||
| "0x{}", | ||
| hex::encode(get_contract_bytecode(erc20_abi_path.to_str().unwrap()).unwrap()) | ||
| ); |
There was a problem hiding this comment.
Same here with the unwraps. We also need to decide if we want the program to just panic and terminate if this fails (which will happen with unwrap and expect etc.) or do we want the user to decide how to handle the error. If we want the user to be able to handle the error we need to return a Result and propagate these internal errors instead of panicking.
I personally think we should return a Result... the user might want to skip that simulation that this is making fail, but keep their program running and simulating non-vm pools.
There was a problem hiding this comment.
I'm not sure I agree with you - I'm thinking about the scenarios where this fails:
- malformed ERC20.abi file
- malformed directory structure
And I am not sure it's beneficial to return a result here - these are not really expected errors that one can just skip for one simulation and expect that the next simulation will be okay. If any one of those things is wrong - it's truly game over for all sims. I do think in this case it's more beneficial to use expect.
Let me know if I'm missing something!
There was a problem hiding this comment.
Hmmm... yeah, I agree with you in terms of vm only simulations. But in rust I tend to lean towards giving the call site/user the power to choose.
E.g. If I'm a small solver that mainly uses native protocols I might decide to just handle this error by ignoring vm protocols and just logging a warning. To me my focus is on keeping my native simulations going.
On the other side, if I rely on vm simulations I can then choose to panic on this error.
I like having the choice.
There was a problem hiding this comment.
Makes more sense! I'll add that in then :)
unwrap should not be used in production code - this hides the true error. Other readability fixes: - Remove redundant comments - Don't use TxHash type alias: introduce SlotHash type alias for readability - Introduce Overwrites alias - since this type will be repeated a lot. - Fix struct docs (the comment should be preceded by a /// and go on top of the attribute)
- Adapted from the python docs. - Made mods public so they can actually be imported (noticed this discrepancy when writing the docs). Not sure if there is a downside to this.
src/protocol/vm/utils.rs
Outdated
| let code = format!( | ||
| "0x{}", | ||
| hex::encode(get_contract_bytecode(erc20_abi_path.to_str().unwrap()).unwrap()) | ||
| ); |
There was a problem hiding this comment.
Hmmm... yeah, I agree with you in terms of vm only simulations. But in rust I tend to lean towards giving the call site/user the power to choose.
E.g. If I'm a small solver that mainly uses native protocols I might decide to just handle this error by ignoring vm protocols and just logging a warning. To me my focus is on keeping my native simulations going.
On the other side, if I rely on vm simulations I can then choose to panic on this error.
I like having the choice.
|
Just realized I should probably also move this to its own file - will do tomorrow! |
- Consider the case of supporting both native and vm protocols. If I'm a small solver that mainly uses native protocols I might decide to just handle this error by ignoring vm protocols and just logging a warning. To me my focus is on keeping my native simulations going. Other changes: - Abolish SlotHash: An outside user will more readily know what type H256 needs from them than our arb type SlotHash. - Docstrings: We are able to get the slot index of any H160 - not just address
For organization.
We want: - Familiar types for the user (H160, H256) - Custom types for us internally (SlotHash, Address) This makes life easier for both us and the user, as everyone is working with something that is clearer to them.
|
This PR is included in version 0.24.0 🎉 |
Issue