3.0.0: Improve object encoding and decoding #862

jasonpaulos · 2024-03-25T20:14:41Z

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 · 2024-03-26T06:07:19Z

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

jasonpaulos · 2024-03-26T13:38:07Z

@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

.test-env

jasonpaulos · 2024-05-10T22:57:30Z

src/client/v2/algod/models/types.ts

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

jasonpaulos · 2024-05-10T22:59:41Z

src/client/v2/indexer/models/types.ts

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

jasonpaulos · 2024-05-10T23:16:02Z

src/multisig.ts

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 · 2024-05-28T18:58:30Z

src/signing.ts

+import { LogicSig, LogicSigAccount } from './logicsig.js';
+import { addressFromMultisigPreImg } from './multisig.js';
+
+function signLogicSigTransactionWithAddress(


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

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

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.

src/boxStorage.ts

src/signedTransaction.ts

gmalouf · 2024-05-28T19:16:53Z

src/signedTransaction.ts

+    if (!(data instanceof Map)) {
+      throw new Error(`Invalid decoded SignedTransaction: ${data}`);
+    }
+    return new SignedTransaction({


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

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).

…st/linguist/blob/master/docs/overrides.md#generated-code

…orking-with-files/managing-files/customizing-how-changed-files-appear-on-github

package-lock.json

src/boxStorage.ts

algorandskiy · 2024-05-29T20:29:45Z

src/client/v2/algod/accountApplicationInformation.ts

  // eslint-disable-next-line class-methods-use-this
  prepare(body: Record<string, any>): AccountApplicationResponse {
-    return AccountApplicationResponse.from_obj_for_encoding(body);
+    return AccountApplicationResponse.fromEncodingData(


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.

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 · 2024-05-29T20:51:39Z

src/dryrun.ts

  await Promise.all(acctPromises);

  return new DryrunRequest({
-    txns: txns.map((st) => ({ ...st, txn: st.txn.get_obj_for_encoding() })),


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

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 · 2024-05-29T20:54:44Z

src/encoding/schema/boolean.ts

+  }
+
+  public isDefaultValue(data: unknown): boolean {
+    return data === false;


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

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 · 2024-05-29T21:01:43Z

src/types/blockHeader.ts

+    ['gh', header.gh],
+    ['prev', header.prev],
+    ['proto', header.proto],
+    ['rate', header.rate],


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

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 · 2024-05-29T21:28:45Z

src/signing.ts

+import { LogicSig, LogicSigAccount } from './logicsig.js';
+import { addressFromMultisigPreImg } from './multisig.js';
+
+function signLogicSigTransactionWithAddress(


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

algorandskiy · 2024-05-29T21:37:36Z

src/signedTransaction.ts

+      ['sgnr', this.sgnr],
+    ]);
+    if (this.msig) {
+      data.set('msig', encodedMultiSigToEncodingData(this.msig));


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

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 · 2024-05-29T21:46:12Z

src/multisigSigning.ts

+  if (!keyExist) {
+    throw new Error(MULTISIG_KEY_NOT_EXIST_ERROR_MSG);
+  }
+


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; }

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;

}

…s place

encodeJson test pending

Improved encoding & decoding work, still work in progress

d003221

jasonpaulos added the Enhancement label Mar 25, 2024

Fix up transactions unit test

6bdc9ba

jasonpaulos mentioned this pull request Mar 27, 2024

Generator changes for improved encoding & decoding algorand/generator#73

Merged

jasonpaulos added 2 commits March 28, 2024 17:30

Fixing type errors

6e80bd6

Update cucumber test branch

71999a6

jasonpaulos commented Mar 28, 2024

View reviewed changes

.test-env Outdated Show resolved Hide resolved

jasonpaulos added 18 commits March 29, 2024 15:07

Fix more type errors

6f58347

Untyped model

312fd65

Fixed untyped value conversions

04af841

Temporarily disable block parsing unit tests

564c889

Fix integration test errors

6637ead

Update examples

6235396

Initial work on unifying msgp and json paths with a schema object

1ecb5e2

Make everything Encodable, including generated models

abcbd9a

Fix most unit tests

d6c4913

Update json decoding references

54aeb78

Passing unit tests

3b99c59

Type issue

3d911fc

Fixing integration test failures

65e0724

fix mistake

381347d

Simplifying code

7e6ca55

Remove BaseModel, introduce tests for Schemas

0a493ae

Fix issues

272df51

Clean up box translation

b11917b

jasonpaulos commented May 10, 2024

View reviewed changes

gmalouf reviewed May 28, 2024

View reviewed changes

src/boxStorage.ts Show resolved Hide resolved

gmalouf reviewed May 28, 2024

View reviewed changes

src/signedTransaction.ts Outdated Show resolved Hide resolved

gmalouf reviewed May 28, 2024

View reviewed changes

src/signedTransaction.ts Show resolved Hide resolved

gmalouf reviewed May 28, 2024

View reviewed changes

gmalouf requested a review from algorandskiy May 29, 2024 17:47

ohill added 2 commits May 29, 2024 15:20

Mark types.ts as generated following https://github.com/github-lingui…

d72e724

…st/linguist/blob/master/docs/overrides.md#generated-code

set linguist-generated=true https://docs.github.com/en/repositories/w…

b864c24

…orking-with-files/managing-files/customizing-how-changed-files-appear-on-github

algorandskiy reviewed May 29, 2024

View reviewed changes

jasonpaulos added 2 commits May 30, 2024 17:53

Remove required from NamedMapEntry & introduce OptionalSchema in it…

f79528e

…s place

Fix integration test error

6c90132

jasonpaulos requested review from algorandskiy and gmalouf June 4, 2024 13:42

gmalouf previously approved these changes Jun 4, 2024

View reviewed changes

jasonpaulos added 4 commits June 4, 2024 11:32

Expose and test encode/decodeJSON

8a29264

Migration guide update

f67acb6

typo

d11db00

docstrings

c82a856

algorandskiy approved these changes Jun 4, 2024

View reviewed changes

jasonpaulos mentioned this pull request Jun 5, 2024

Excluding empty values algorand/go-algorand-sdk#269

Closed

Update .test-env

86a6e11

jasonpaulos requested a review from gmalouf June 5, 2024 17:21

gmalouf approved these changes Jun 5, 2024

View reviewed changes

jasonpaulos merged commit f9013e2 into 3.0.0 Jun 5, 2024

jasonpaulos deleted the improved-encoding branch June 5, 2024 17:24

jasonpaulos mentioned this pull request Jun 11, 2024

Fix EncodedSignedTransaction decoding from JSON #849

Closed

jasonpaulos mentioned this pull request Aug 21, 2024

FOR REVIEW ONLY: js-algorand-sdk v3.0.0-beta.1 #891

Closed

onetechnical mentioned this pull request Sep 12, 2024

FOR REVIEW ONLY: js-algorand-sdk v3.0.0 #901

Merged

	// 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;
	}

3.0.0: Improve object encoding and decoding #862

3.0.0: Improve object encoding and decoding #862

Uh oh!

Conversation

jasonpaulos commented Mar 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emg110 commented Mar 26, 2024

Uh oh!

jasonpaulos commented Mar 26, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonpaulos May 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmalouf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jasonpaulos commented Mar 25, 2024 •

edited

Loading

jasonpaulos May 10, 2024 •

edited

Loading