3.0.0: Improve object encoding and decoding

[jasonpaulos]

• Upgrade to the newly released algorand-msgpack library
• Remove the from_obj_for_encoding and get_obj_for_encoding methods. In their place, introduce the following:
  • An Encodable interface with a toEncodingData method, and a static fromEncodingData method. "Encoding data" is an intermediate representation of the object in a JS Map, similar to the output of our Python SDK's dictify method.
  • The Encodable interface also has a getEncodingSchema method and a static encodingSchema property, which return instances of the new Schema base class.
  • The Schema class is how "encoding data" gets transformed for encoding/decoding to/from JSON and msgpack. For example, AddressSchema converts an Address class to its 32-byte public key for encoding in msgpack, and for JSON it converts the address to its base32 string form. Specifically these translations happen in the prepareMsgpack/fromPreparedMsgpack and prepareJSON/fromPreparedJSON methods.
• Replace EncodedSignedTransaction interface with SignedTransaction class

While the introduction of Schema adds some complexity, it also has these benefits:

• Consolidating encoding decisions which otherwise must be spread across every class's encoding/decoding functions
• Reducing mistakes which can be made in each implementing class. No longer do they need to worry about omitting empty values or correctly dealing with base64 encoding for JSON. The toEncodingData and fromEncodingData can be more straightforward translations now.
• Real, configurable support for omitting empty values. The NamedMapSchema class determines if empty values should be omitted and seamlessly restores omitted values when decoding.
  • Each map field has a boolean property omitEmpty which, if true, omits default values during encoding.
  • When decoding, if a field has been omitted, sometimes we don't want to restore it to that field's default value, we'd rather just know if it wasn't present. For this reason there is the OptionalSchema which can wrap another schema. It will restore omitted values to undefined.
• Opens up the possibility of using non-string keys, which are necessary to support blocks and stateproof transactions (to be addressed in a later PR)

This relies on generator changes as well from algorand/generator#73

[emg110]

Hey @jasonpaulos, some of these look not to be backward compatible and to some extent breaking changes. Are they?

[jasonpaulos]

@emg110 this is still a work in progress, but yes, there are breaking change here. That's why this is going into the v3 branch

[jasonpaulos]

As a reminder, this file is generated, and its changes are a result of algorand/generator#73

[jasonpaulos]

As a reminder, this file is generated, and its changes are a result of algorand/generator#73

[jasonpaulos]

These changes are mostly to move code which relies on SignedTransaction (i.e. code which produces signed msig txns) to src/multisigSigning.ts.

Otherwise there's be a circular dependency, because src/signedTransaction.ts depends on this file

[gmalouf]

Not positive this is covered by testing (pre-existing).

[algorandskiy]

appears to be covered as signLogicSigTransaction -> signLogicSigTransactionObject -> signLogicSigTransactionObject but not sure if all cases are covered

[jasonpaulos]

This is an internal function, so it's not tested directly. But as Pavel pointed out, since every call to signLogicSigTransaction and signLogicSigTransactionObject resolves to this, and those are covered well here, this should have good coverage.

[gmalouf]

should there be a check for more than one signature on decode?

[jasonpaulos]

I'm not so sure about this. If you try to create a signed txn with more than one signature, then yes we should error and stop you because you're doing something wrong and it's best to fail early.

But if you're just trying to read such a transaction that someone else created, I don't see a clear benefit to erroring in that case. There are almost an unlimited number of ways you can invalidate a transaction, and I'm not sure it's worth the effort to try to validate every field completely upon decoding (we do validate that field types are correct, but field values/consistency is another thing entirely). That will be done when you send the transaction to a node for processing anyway.

[gmalouf]

Left a few asks, overall makes sense and is definitely cleaner/seemingly more sustainable. Some concern about exported functions not covered by tests (even before these changes).

[algorandskiy]

imo this interface from user point of view is too verbose. Can't fromEncodingData check if it is getting raw json or a encodingSchema-compatible dict ?
This pattern repeats over and over and over except few places where dicts supplied into fromEncodingData.
Well, unless all these verboseness is only in auto-generated and not exposed to end user.

[jasonpaulos]

Yes this is pretty verbose and not very nice.

When decoding from a serialized JSON string, decodeJSON can be used, e.g. decodeJSON(str, AccountApplicationResponse). This is what I expect users will need the vast majority of the time, not the verbose code present in the request classes.

In these request classes, we are in a bit of weird situation when prepare function is called, since the JSON response is already parsed into an object. I have a desire to change this so that the JSON string is the input to prepare, then decodeJSON can be used and the code would be cleaner. But this PR has already grown large, so I didn't want to attempt that now.

[algorandskiy]

Am I right and the generated code for DryrunRequest serialization on sending expects it (and all its props) to recursive Encodable ?

[jasonpaulos]

Not really, that's more how the old code work, where get_obj_for_encoding was implemented generically by BaseModel and it attempted to call get_obj_for_encoding on all field values. See this file from develop: https://github.com/algorand/js-algorand-sdk/blob/981905ad8511cea706fc77d0c133bb1962f76927/src/client/v2/basemodel.ts

With the changes in this PR, it's more like DryrunRequest knows which of its fields are Encodable and it will call toEncodingData on them when its own toEncodingData method is called. There is no generic searching all objects for the magic function name.

[algorandskiy]

maybe undefined us also good enough default boolean (so that omitted from encoding) ?

[jasonpaulos]

NamedMapSchema is where omitting empty values is implemented, and it works without needing to allow undefined here.

Separately, I recently introduced an OptionalSchema that you can wrap around any other schema to allow undefined values. It doesn't change how maps omit empty values, but it does allow for values to be decoded as undefined, which is useful in a language like JS without any native notion of zero values.

[algorandskiy]

missing TxnCounter uint64 codec:"tc" missing `RewardsLevel uint64 `codec:"earn" from rewardState, not sure what is the rule here.
also missing partupdrmv and all UpgradeState fields except proto

[jasonpaulos]

Yes this block header representation is very outdated and wrong. We have an issue to fix in #846, and since this PR is large I didn't want to address that now. I only preserved the existing structure, even though it needs improvement

[algorandskiy]

appears to be covered as signLogicSigTransaction -> signLogicSigTransactionObject -> signLogicSigTransactionObject but not sure if all cases are covered

[algorandskiy]

wouldn't it retain the empty sig property if msig provided?

[jasonpaulos]

If msig is defined then sig must be undefined. Undefined values are dropped during msgpack and JSON preparation, so it makes no difference.

A previous version of the code looked something like:

if (this.sig) {
    data.set('sig', this.sig)
}
if (this.msig) {
    data.set('msig', encodedMultiSigToEncodingData(this.msig))
}

Due to dropping undefined values this is equivalent to the current code.

[algorandskiy]

this misses msig check compare to the original code

  const msigAddr = addressFromMultisigPreImg({
    version,
    threshold,
    pks,
  });
  const senderAddr = signedTxn.txn.snd
    ? new Address(signedTxn.txn.snd)
    : Address.zeroAddress();
  if (!senderAddr.equals(msigAddr)) {
    signedTxn.sgnr = msigAddr.publicKey;
  }

[jasonpaulos]

Yes, I removed this from createMultisigTransactionWithSignature because it's already checked in createMultisigTransaction, which is called at the beginning of the function.

createMultisigTransaction:

js-algorand-sdk/src/multisigSigning.ts

Lines 54 to 64 in b864c24

	// if the address of this multisig is different from the transaction sender,
	// we need to add the auth-addr field
	const msigAddr = addressFromMultisigPreImg({
	version,
	threshold,
	pks,
	});
	let sgnr: Address | undefined;
	if (!txn.sender.equals(msigAddr)) {
	sgnr = msigAddr;
	}

encodeJson test pending