algebra icon indicating copy to clipboard operation
algebra copied to clipboard

Miscellaneous Changes: Serialization, additional helper methods, reorg

Open jon-chuang opened this issue 4 years ago • 3 comments

Description

Miscelaneous changes associated with celo-org merge. Ranging from harmless to API changes. closes: https://github.com/arkworks-rs/algebra/pull/127


Before we can merge this PR, please make sure that all the following items have been checked off. If any of the checklist items are not applicable, please leave them but write a little note why.

  • [x] Targeted PR against correct branch (master)
  • [x] Linked to Github issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • [x] Wrote unit tests
  • [x] Updated relevant documentation in the code
  • [ ] Added a relevant changelog entry to the Pending section in CHANGELOG.md
  • [ ] Re-reviewed Files changed in the Github PR explorer

jon-chuang avatar Mar 17 '21 11:03 jon-chuang

To reviewers:

  • Not sure if you'd like for me to refactor further. This is the least cohesive PRs of those to come since I've staged them to build atop one another, and this is the remaining changes that don't have any specific topic it is tied to. However, if you think it would be cleaner to extract the diff of one of the branches jonch/celo-org-... feel free to let me know or to do so yourself.

jon-chuang avatar Mar 17 '21 11:03 jon-chuang

For the serialize changes, we should update the derive macro to additionally incorporate uncompressed_unchecked.

Also, maybe in a follow-up PR, let's make it so that the Serialize and Deserialize traits both have a (de)serialize_with_mode method:

trait CanonicalSerialize {
...
	fn serialize_with_mode<W: Write>(&self, compression: ShouldCompress, writer: W) {
		match compression {
			ShouldCompress::Yes => {
				// do stuff
			},
			ShouldCompress::No => {
				// do other stuff
			}
		}
	}

	// all other methods invoke `serialize_with_mode` with appropriate modes.
}

trait CanonicalDeserialize {
	fn deserialize_with_mode<R: Read>(&self, compression: ShouldCompress, check: ShouldCheck reader: R) -> Result<Self> {
		match compression {
			ShouldCompress::Yes => {
				// do stuff
			},
			ShouldCompress::No => {
				// do other stuff
			}
		}
		if let ShouldCheck::Yes = check {
			// perform checks
		}
		Ok(result)
	}
	
	// all other methods invoke `deserialize_with_mode` with appropriate modes.
}

(I omitted the check from serialize, because AFAIK there are no checks performed during serialization? This means that we remove serialize_unchecked and serialize_uncompressed_unchecked too.)

The reason why we want to add the above API is so that downstream implementers can just implement the logic in one method, instead of in four different ones.

Pratyush avatar Mar 17 '21 12:03 Pratyush

@jon-chuang is there anything I can do to help push this forward?

Pratyush avatar Mar 22 '21 05:03 Pratyush