Eth-connector: merge develop branch#109
Conversation
* Validate register length in `read_input_arr20()` * Only read register length in `Engine::get_code_size` * Add `read_input_borsh()` * Ensure `method.args.len() == args_decoded.len()` * Ensure register size is 8 in `read_u64` * Use constant to specify the register ID used in `read_input()`
…gine into eth-connector-merge-develop
sept-en
added
C-enhancement
birchmd
left a comment
birchmd
left a comment
There was a problem hiding this comment.
Nice job!
I think we can remove most of the TODOs you called out. The only thing I think needs additional attention is refactoring internal_set_eth_balance to accept Wei, but we could do that as a follow-up PR if we are in a hurry to merge this one.
| sdk::read_u64(&Self::get_statistic_key()).unwrap_or(0) | ||
| sdk::read_u64(&Self::get_statistic_key()) | ||
| .unwrap_or(Ok(0)) | ||
| .unwrap_or(0) |
| #[global_allocator] | ||
| static ALLOC: wee_alloc::WeeAlloc = wee_alloc::WeeAlloc::INIT; | ||
|
|
||
| #[cfg(target_arch = "wasm32")] |
There was a problem hiding this comment.
Why did it move up top of contract feature?
There was a problem hiding this comment.
It's needed for a test contract which uses the production contract as a dependency, but I can't have the production public exports (i.e. contract feature) because I want to override the definition in the test contract and you can't export a two methods with the same name.
| exports::input(0); | ||
| let bytes: Vec<u8> = vec![0; exports::register_len(0) as usize]; | ||
| exports::read_register(0, bytes.as_ptr() as *const u64 as u64); | ||
| exports::input(INPUT_REGISTER_ID); |
There was a problem hiding this comment.
@birchmd IMHO all those changes really redundant.
WASM has an infinity register stack. We don't need to unify as some standard get/read/write register flow. Most important read from the same register. And it was. it will be misleading that we should only use one given register.
There was a problem hiding this comment.
I think giving constants a name is useful for two reasons:
- It provides a semantic meaning to an otherwise ambiguous value (
0could mean lots of things, but here it is a register ID) - It makes changing the value less error prone in the future because all occurrences of it now point back to one definition. For example, if we decide to reserve register 0 for some purpose then we can change
INPUT_REGISTER_IDto be some other value easily without carefully double-checking we changed all the right places.
| pub fn read_u64(key: &[u8]) -> Option<u64> { | ||
| unsafe { | ||
| if exports::storage_read(key.len() as u64, key.as_ptr() as u64, 0) == 1 { | ||
| pub(crate) fn read_u64(key: &[u8]) -> Option<Result<u64, InvalidU64>> { |
There was a problem hiding this comment.
@birchmd I think we should return only Result type and remove Option. It's not informative. We can return Err(...) for the first part of the contract too.
5 participants
Merge
developbranch & resolve conflicts.