Canonical address length bumped to 54, max bumped to 64 #995

lukerhoads · 2021-07-06T22:29:05Z

No description provided.

lukerhoads · 2021-07-06T22:29:53Z

I do not know why it is failing so many tests :((((((

webmaster128 · 2021-07-07T13:41:13Z

I do not know why it is failing so many tests :((((((

There are multiple smaller issues, one is just formatting. Start by running the following commands locally:

./devtools/check_workspace.sh
./devtools/check_contracts_medium.sh
./devtools/test_workspace.sh

That should point you to the most obvious problems.

webmaster128 · 2021-07-07T13:44:35Z

In packages/vm/src/imports.rs the line

let dest_ptr = create_empty(&mut instance, 50);

creates a memory region for the human address result. This is too short now. Change the 50 to 70 to make the tests pass. This is done 4x in the same file.

lukerhoads · 2021-07-07T15:47:41Z

Ahh it is a wasm error with windows, I tried changing CONTRACT to directly reference hackatom_0.15.wasm but it says that the target windows is not yet supported so I am going to commit your suggestions as the tests will most likely pass on circleci, lets see

webmaster128 · 2021-07-07T16:12:31Z

Ah, you are on Windows. Then this is a bit tricky. We support developing smart contracts on Windows (i.e. the guest), but not developing the cosmwas-vm (i.e. the host). This is because our Wasm backend (singlepass) does not support Windows yet.

Anyways, you can run cargo fmt directly to get rid of the formatting complains. Running it in the repo root will format the workspace. For the contracts you need to run it in ./contracts/xyz.

lukerhoads · 2021-07-07T17:10:26Z

whoops, this time i need to look directly at circleci errors. let me fix that right now

webmaster128 · 2021-07-07T18:07:09Z

packages/schema/src/remove.rs

@@ -37,7 +37,7 @@ pub fn remove_schemas(schemas_dir: &path::Path) -> Result<(), io::Error> {
 #[cfg(test)]
 mod tests {
    use super::*;
-    use std::ffi::OsStr;
+    // use std::ffi::OsStr;


Why this change? Seems unrelatet and causes compile errors.

that was a formatting thing, my bad gonna revert that.

another question... and this is a stupid rust beginner question but since im on windows does

let source_ptr = write_data(&env, &[61; 33]);

the 33 here needs to be increased, not the 61 right? Testing locally returns the windows contract error

lukerhoads · 2021-07-07T19:03:51Z

And let invalid_bech32 = "bn93hg934hg08q340g8u4jcau3"; this is erroring because this address is successfully canonicalized (as it is in the right range) and it expects an error. so would this have to be increased in length as well?

lukerhoads · 2021-07-10T15:09:52Z

Ok going to fix these last few errors today

lukerhoads · 2021-07-11T04:50:10Z

This commit still does not fix the issue in the staking module, IDK why its throwing the 'VM error: RuntimeErr { msg: "Wasmer runtime error: RuntimeError: Error in guest/host communication: Region too small. Got 32, required 54" }', /root/cosmwasm/packages/vm/src/testing/calls.rs:136:49, let me try to look into it more though I guess haha

lukerhoads · 2021-07-11T04:52:34Z

oh darn, this attempted fix actually failed more tests 😖

lukerhoads · 2021-07-11T20:43:12Z

having issues with wsl for testing i am a trainwreck

lukerhoads · 2021-07-12T04:36:18Z

I am struggling with these tests :///
starts I have are

const DEFAULT_MEMORY_LIMIT: Option<Size> = Some(Size::mebi(16));

this memory limit applied to a mock instance is somehow not enough for the staking contract to call with the new canonical address length?

and the other failures in the package vm are somehow related to the instantiated array

let source_ptr = write_data(&env, &[61; 33]);

which should be longer? im not good enough at rust to really understand them but I feel I was close haha
could I please get some assistance @webmaster128

sorry to bother, would really appreciate it,
thanks

uint · 2021-07-12T11:24:55Z

packages/vm/src/imports.rs

@@ -1190,7 +1190,7 @@ mod tests {
        let (env, mut instance) = make_instance(api);

        let source_ptr = write_data(&env, &[61; 33]);


Suggested change

let source_ptr = write_data(&env, &[61; 33]);

let source_ptr = write_data(&env, &[61; 65]);

Something like [42; 5] means an array filled with five 42s, so basically [42, 42, 42, 42, 42].

Here we're testing that we get an appropriate error if there is too much data. Since you increased the limit and length 33 is now OK, you want to change it to something that should cause an error.

You'll also want to change the assertions below (lines 1206-1207) to match the new length and max length.

uint · 2021-07-12T11:33:28Z

packages/vm/src/imports.rs

@@ -994,7 +994,7 @@ mod tests {
        let api = MockApi::default();
        let (env, _instance) = make_instance(api);

-        let source_ptr = write_data(&env, &[61; 100]);
+        let source_ptr = write_data(&env, &[61; 33]);


Is this change needed? This test seemed perfectly OK before.

uint · 2021-07-12T11:37:32Z

packages/vm/src/imports.rs

@@ -1087,7 +1087,7 @@ mod tests {
        let api = MockApi::default();
        let (env, mut instance) = make_instance(api);

-        let source_ptr = write_data(&env, &[61; 100]);


Same as for do_addr_validate_fails_for_large_inputs - I don't think this is what you wanted to do?

uint · 2021-07-12T12:59:45Z

Let's figure this out! Regarding the vm, I submitted suggestions that should help fix this up.

const DEFAULT_MEMORY_LIMIT: Option<Size> = Some(Size::mebi(16));
this memory limit applied to a mock instance is somehow not enough for the staking contract to call with the new canonical address length?

I don't think that's the line you want. That's the overall limit for the total size of an instance of a contract, and it should be plenty.

I think this is what you want to update:
https://github.com/CosmWasm/cosmwasm/blob/main/packages/std/src/imports.rs#L21

Looks like this is how much space will be allocated to store the canonical address, and with your changes it's too small.

lukerhoads · 2021-07-12T14:53:34Z

Thanks so much for the patience and suggestions. I really appreciate it, probably wouldve taken me another year to figure this out.
Seeing all of the tests pass brings tear to my eyes 😂

packages/std/src/imports.rs

packages/vm/src/imports.rs

Co-authored-by: Simon Warta <2603011+webmaster128@users.noreply.github.com>

webmaster128

Code looks good to me, thanks. Could you just add those CHANGELOG entries to the ### Changed section of ## [Unreleased]:

- cosmwasm-std/cosmwasm-vm: Increase canonical address lengths up to 64 bytes.
- cosmwasm-std/cosmwasm-vm: In `MockApi`, increase max length of supported human
  addresses from 24 bytes to 54 bytes by using a longer canonical
  representation. This allows you to insert typical bech32 addresses in tests.
  ([#995])

[#995]: https://github.com/CosmWasm/cosmwasm/pull/995

CHANGELOG.md

webmaster128

Looks good to me. Thank you!

@uint anything to add?

uint · 2021-07-12T21:35:47Z

Looks good to me. Thank you!

@uint anything to add?

Nope, looks good! Thanks for contributing, @lukerhoads!

Canonical address length bumped to 54

6bc0af2

lukerhoads mentioned this pull request Jul 6, 2021

mock_dependencies - creating canonical_address with long strings + alternate canonical_length #985

Closed

Fixed inadequate memory allocation in create_empty function

803edac

Re formatted root and all contracts

15ad243

webmaster128 reviewed Jul 7, 2021

View reviewed changes

Revert and modified tests in imports

6abdd2f

Attempted fix to package_vm and contract_hackatom testing errors

aa5feee

lukerhoads added 3 commits July 10, 2021 23:58

Reverted attempted VM test fix and fixed hackatom contract test

8f67cee

Actual revert to 33 length

b1d5754

cargo fmt

cf14490

Formatting in hackatom

3a378d4

uint requested changes Jul 12, 2021

View reviewed changes

Implemented suggestions

e5eefe2

uint reviewed Jul 12, 2021

View reviewed changes

packages/std/src/imports.rs Outdated Show resolved Hide resolved

webmaster128 reviewed Jul 12, 2021

View reviewed changes

packages/vm/src/imports.rs Outdated Show resolved Hide resolved

Update packages/vm/src/imports.rs

1270309

Co-authored-by: Simon Warta <2603011+webmaster128@users.noreply.github.com>

lukerhoads added 2 commits July 12, 2021 10:55

Buffer length bumped to 64 to allow flexibility

09880a8

Merge branch 'main' of https://github.com/lukerhoads/cosmwasm into main

8c51226

webmaster128 reviewed Jul 12, 2021

View reviewed changes

Changelog entries

6ae035c

webmaster128 reviewed Jul 12, 2021

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

Update CHANGELOG.md

8e2f579

webmaster128 approved these changes Jul 12, 2021

View reviewed changes

uint approved these changes Jul 12, 2021

View reviewed changes

webmaster128 merged commit bf68ab6 into CosmWasm:main Jul 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Canonical address length bumped to 54, max bumped to 64 #995

Canonical address length bumped to 54, max bumped to 64 #995

lukerhoads commented Jul 6, 2021

lukerhoads commented Jul 6, 2021

webmaster128 commented Jul 7, 2021

webmaster128 commented Jul 7, 2021 •

edited

Loading

lukerhoads commented Jul 7, 2021

webmaster128 commented Jul 7, 2021

lukerhoads commented Jul 7, 2021

webmaster128 Jul 7, 2021

lukerhoads Jul 7, 2021

lukerhoads commented Jul 7, 2021

lukerhoads commented Jul 10, 2021

lukerhoads commented Jul 11, 2021

lukerhoads commented Jul 11, 2021

lukerhoads commented Jul 11, 2021

lukerhoads commented Jul 12, 2021

uint Jul 12, 2021

uint Jul 12, 2021

uint Jul 12, 2021

uint Jul 12, 2021

uint commented Jul 12, 2021 •

edited

Loading

lukerhoads commented Jul 12, 2021

webmaster128 left a comment

webmaster128 left a comment

uint commented Jul 12, 2021

		@@ -1190,7 +1190,7 @@ mod tests {
		let (env, mut instance) = make_instance(api);

		let source_ptr = write_data(&env, &[61; 33]);

Canonical address length bumped to 54, max bumped to 64 #995

Canonical address length bumped to 54, max bumped to 64 #995

Conversation

lukerhoads commented Jul 6, 2021

lukerhoads commented Jul 6, 2021

webmaster128 commented Jul 7, 2021

webmaster128 commented Jul 7, 2021 • edited Loading

lukerhoads commented Jul 7, 2021

webmaster128 commented Jul 7, 2021

lukerhoads commented Jul 7, 2021

webmaster128 Jul 7, 2021

Choose a reason for hiding this comment

lukerhoads Jul 7, 2021

Choose a reason for hiding this comment

lukerhoads commented Jul 7, 2021

lukerhoads commented Jul 10, 2021

lukerhoads commented Jul 11, 2021

lukerhoads commented Jul 11, 2021

lukerhoads commented Jul 11, 2021

lukerhoads commented Jul 12, 2021

uint Jul 12, 2021

Choose a reason for hiding this comment

uint Jul 12, 2021

Choose a reason for hiding this comment

uint Jul 12, 2021

Choose a reason for hiding this comment

uint Jul 12, 2021

Choose a reason for hiding this comment

uint commented Jul 12, 2021 • edited Loading

lukerhoads commented Jul 12, 2021

webmaster128 left a comment

Choose a reason for hiding this comment

webmaster128 left a comment

Choose a reason for hiding this comment

uint commented Jul 12, 2021

webmaster128 commented Jul 7, 2021 •

edited

Loading

uint commented Jul 12, 2021 •

edited

Loading