Skip to content
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

Fix tests to remove address_to_object_id_hack hack #23

Merged
merged 10 commits into from
Dec 7, 2021

Conversation

patrickkuo
Copy link
Contributor

@patrickkuo patrickkuo commented Dec 3, 2021

  • removed unused amount args
  • renamed init_state_with_account to init_state_with_object
  • added get_object_id function to generate random object id for testing

removing the rest of "address_to_object_id_hack" from client.rs will be more involve and require adding object_id(s?) to ClientState, I will keep this PR short and do it in part 2

@patrickkuo patrickkuo self-assigned this Dec 3, 2021
Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One quick comment, hope this helps.

I recommend setting a comfortable environment where running cargo fmt is easy for you on any commit, perhaps with a git pre-commit hook or an IDE setup?

@@ -53,6 +54,10 @@ pub fn address_to_object_id_hack(address: FastPayAddress) -> ObjectID {
.expect("An address is always long enough to extract 20 bytes")
}

pub fn random_object_id() -> ObjectID {
Copy link
Contributor

@huitseeker huitseeker Dec 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, in production we're going to need this to be 100% deterministic, based on a set of inputs (e.g. a hash of the object bytes + the Tx creating it, etc).

The reason for this is that the output of the computation of distinct machines on the network needs to be the same, as the success of a unit of computation will depend on a pool of machines producing a collective cryptographic signature on that output. They need to sign "the same bytes".

What you have would be fine for local unit tests, though.

Here's a set of ideas on how to go about this while we're figuring out how to do this:

  • conditional compilation can let this function have different code (hence meaning) when running tests and in production,
  • the rand Rust crate contains the RNGs we're looking for,
  • therein, the SeedableRng trait captures the kinds of Rng we would pick here, and I'd recommend using an API like it to generate the ObjectID: in production, every ObjectID is generated from runtime seed bytes (TBD) + a PRNG, with the details of the later being set at compilation time.

If you go look into the rand crate, a tip if not to change its version (which is not the latest) for right now (for reasons disconnected to this PR).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

This method is intended to be use in testing only, I should move it out of the core package

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also prepend the function with #[cfg(test)] and it will only be included when compiling tests.

@gdanezis gdanezis linked an issue Dec 3, 2021 that may be closed by this pull request
@patrickkuo patrickkuo force-pushed the pat/object_id_hack_refactoring branch from 23e7fac to f296c62 Compare December 4, 2021 23:09
@patrickkuo
Copy link
Contributor Author

patrickkuo commented Dec 4, 2021

removing the rest of "address_to_object_id_hack" from client.rs will be more involve and require adding object_id(s?) to ClientState, I will keep this PR short and do it in part 2

@patrickkuo patrickkuo marked this pull request as ready for review December 4, 2021 23:31
@patrickkuo patrickkuo force-pushed the pat/object_id_hack_refactoring branch from 9b4db1d to ba10c4c Compare December 6, 2021 13:18
Copy link
Collaborator

@sblackshear sblackshear left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@patrickkuo patrickkuo merged commit c36068f into main Dec 7, 2021
@patrickkuo patrickkuo deleted the pat/object_id_hack_refactoring branch December 8, 2021 23:22
brson pushed a commit to brson/sui that referenced this pull request Apr 30, 2024
* 041924-expected_failure-with-params: ini

* 041924-expected_failure-with-params: clean-up
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[fastx distsys] Fix tests to remove address_to_object_id_hack hack
4 participants