3.0.0: Refactor `Transaction` class #854

jasonpaulos · 2024-01-25T16:41:21Z

Specifically,

For the Transaction class, separated input type from internal representation (e.g. input types can be number | bigint, but internally the type is stored as bigint). Fixes Refactor Transaction class #850
Greatly simplified types related to transactions and transaction params
Removed all transaction maker methods that did not take objects (e.g. makePaymentTxnWithSuggestedParams(from, to, amount, closeRemainderTo, note, suggestedParams, rekeyTo) replaced by makePaymentTxnWithSuggestedParamsFromObject(object))
Removed "raw transaction objects" (i.e. raw objects with transactions properties like { from: addr, to: adder, ... })
Merged SuggestedParams and SuggestedParamsWithMinFee. Min fee is now required in SuggestedParams
Introduce Address class. Prior to this, addresses were represented internally in either their string form, or a deconstructed object containing public key and checksum. The new Address class standardizes the format used, and internally it only contains the 32 byte public key component
Removed unnecessary MultisigTransaction and TxGroup classes
Removed auction bids which were only relevant to mainnet launch

Additionally I started a v2 to v3 migration guide

Follow up to #853

jasonpaulos · 2024-02-07T18:38:13Z

.circleci/config.yml

@@ -89,7 +89,8 @@ jobs:
              $(lsb_release -cs) stable" | $SUDO tee /etc/apt/sources.list.d/docker.list > /dev/null
            $SUDO apt update
            $SUDO apt -y install docker-ce docker-ce-cli containerd.io
-      - browser-tools/install-browser-tools
+      - browser-tools/install-browser-tools:
+          replace-existing-chrome: true


This ensures we always get the latest stable chrome version, which our browser testing relies on

Is there a reason to intentionally test on a cross-range of versions? Wondering if instead of doing latest, we should do latest, plus X releases back.

There may be some value in selecting an older chrome version to test against, so that we can be sure the code isn't relying on APIs/behaviors that we think are too new.

But in reality it's easiest to test against latest and we still benefit a lot from having this.

I only included this change because for some reason circle stopped installing the latest chrome by default, which broke our test

jannotti

Some thoughts and questions.

examples/atomics.ts

examples/overview.ts

examples/participation.ts

src/boxStorage.ts

src/composer.ts

src/encoding/address.ts

src/client/v2/algod/getApplicationByID.ts

src/client/v2/algod/suggestedParams.ts

src/composer.ts

gmalouf

Initial set of comments, about 20 files in.

gmalouf · 2024-02-07T20:39:12Z

.circleci/config.yml

@@ -89,7 +89,8 @@ jobs:
              $(lsb_release -cs) stable" | $SUDO tee /etc/apt/sources.list.d/docker.list > /dev/null
            $SUDO apt update
            $SUDO apt -y install docker-ce docker-ce-cli containerd.io
-      - browser-tools/install-browser-tools
+      - browser-tools/install-browser-tools:
+          replace-existing-chrome: true


Is there a reason to intentionally test on a cross-range of versions? Wondering if instead of doing latest, we should do latest, plus X releases back.

gmalouf · 2024-02-07T20:44:27Z

examples/atomics.ts

@@ -51,13 +51,12 @@ async function main() {
  // example: ATOMIC_GROUP_SEND

  // example: CONST_MIN_FEE
-  const minFee = algosdk.ALGORAND_MIN_TX_FEE;
-  console.log(minFee);
+  // This SDK does not expose a constant for the minimum fee
  // example: CONST_MIN_FEE


Should we remove the comment references to CONST_MIN_FEE entirely? Also, there seems to be two identical lines for it.

These comments are used to populate snippets of code in the dev site. This specific one is used here: https://developer.algorand.org/docs/get-details/dapps/smart-contracts/guidelines/#get-minimum-fee-off-chain-with-sdk

I think we want to preserve JS in that tab, but just have a comment saying a hard coded min fee is not available. That's currently what happens for the "using an algod API" JS snippet on that page

examples/overview.ts

src/boxStorage.ts

src/composer.ts

src/dryrun.ts

gmalouf

With this large of a refactor, found myself wishing we had a test coverage tool on the SDK. It was hard for me to track down if the varying types were handled properly etc and if we had added new tests in all the places they made sense.

v2_TO_v3_MIGRATION_GUIDE.md

src/utils/utils.ts

gmalouf · 2024-02-08T16:49:44Z

tests/6.Multisig.ts

@@ -478,235 +487,4 @@ describe('Multisig Functionality', () => {
      }, new Error('Invalid multisig transaction, multisig structure missing at index 1'));
    });
  });
-
-  describe('read-only transaction methods should work as expected on multisig transactions', () => {


what was removed that made these irrelevant?

The MultisigTransaction class was removed

src/makeTxn.ts

algorandskiy

In general LGTM, not sure I am following the number | bigint vs bigint logic. Plus, few questions/remarks.

examples/participation.ts

src/client/v2/algod/compile.ts

src/makeTxn.ts

tests/cucumber/steps/steps.js

tests/5.Transaction.ts

algorandskiy

👍

jasonpaulos force-pushed the refactor-transaction branch 2 times, most recently from a4f56c1 to 13e0add Compare February 2, 2024 19:29

jasonpaulos added 5 commits February 5, 2024 10:23

Refactor transaction class

476902c

Fix integration tests

ced00e0

Introduce Address class

be5e469

export multisig step fix

63990dc

other multisig step fix

790e95f

jasonpaulos force-pushed the refactor-transaction branch from 13e0add to 790e95f Compare February 5, 2024 15:24

jasonpaulos added 8 commits February 6, 2024 13:20

Small tweaks, and add migrate guide

e630d7b

missed change from before

034a7c4

Remove unnecessary renames

11331aa

Fix examples

254a9a5

Update circle browser tools

6d9b5ae

Replace existing chrome

804ccf2

Redo parameter rename

2e51a5d

Make genesis hash optional and Uint8Array

93a7ea9

jasonpaulos marked this pull request as ready for review February 7, 2024 17:17

jasonpaulos requested a review from a team as a code owner February 7, 2024 17:17

jasonpaulos requested review from onetechnical, jannotti, gmalouf and algorandskiy February 7, 2024 17:17

jasonpaulos commented Feb 7, 2024

View reviewed changes

jannotti reviewed Feb 7, 2024

View reviewed changes

gmalouf reviewed Feb 7, 2024

View reviewed changes

gmalouf reviewed Feb 8, 2024

View reviewed changes

jasonpaulos added 2 commits February 8, 2024 13:14

Code review feedback

ba432ca

Add unit tests for new utils functions

9d205bd

algorandskiy reviewed Feb 8, 2024

View reviewed changes

examples/participation.ts Outdated Show resolved Hide resolved

src/client/v2/algod/compile.ts Show resolved Hide resolved

src/makeTxn.ts Show resolved Hide resolved

tests/cucumber/steps/steps.js Show resolved Hide resolved

tests/5.Transaction.ts Outdated Show resolved Hide resolved

remove console.log in test

ffc585b

simplify participation example math

1e3371a

gmalouf approved these changes Feb 13, 2024

View reviewed changes

jasonpaulos merged commit 4184324 into 3.0.0 Feb 13, 2024
2 checks passed

jasonpaulos deleted the refactor-transaction branch February 13, 2024 20:58

algorandskiy reviewed Feb 13, 2024

View reviewed changes

jasonpaulos mentioned this pull request Feb 14, 2024

Refactor Transaction class #850

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

3.0.0: Refactor `Transaction` class #854

3.0.0: Refactor `Transaction` class #854

jasonpaulos commented Jan 25, 2024 •

edited

Loading

jasonpaulos Feb 7, 2024

gmalouf Feb 7, 2024 •

edited

Loading

jasonpaulos Feb 8, 2024

jannotti left a comment

gmalouf left a comment

gmalouf Feb 7, 2024 •

edited

Loading

gmalouf Feb 7, 2024

jasonpaulos Feb 8, 2024

gmalouf left a comment

gmalouf Feb 8, 2024

jasonpaulos Feb 8, 2024

algorandskiy left a comment

algorandskiy left a comment

3.0.0: Refactor Transaction class #854

3.0.0: Refactor Transaction class #854

Conversation

jasonpaulos commented Jan 25, 2024 • edited Loading

jasonpaulos Feb 7, 2024

Choose a reason for hiding this comment

gmalouf Feb 7, 2024 • edited Loading

Choose a reason for hiding this comment

jasonpaulos Feb 8, 2024

Choose a reason for hiding this comment

jannotti left a comment

Choose a reason for hiding this comment

gmalouf left a comment

Choose a reason for hiding this comment

gmalouf Feb 7, 2024 • edited Loading

Choose a reason for hiding this comment

gmalouf Feb 7, 2024

Choose a reason for hiding this comment

jasonpaulos Feb 8, 2024

Choose a reason for hiding this comment

gmalouf left a comment

Choose a reason for hiding this comment

gmalouf Feb 8, 2024

Choose a reason for hiding this comment

jasonpaulos Feb 8, 2024

Choose a reason for hiding this comment

algorandskiy left a comment

Choose a reason for hiding this comment

algorandskiy left a comment

Choose a reason for hiding this comment

3.0.0: Refactor `Transaction` class #854

3.0.0: Refactor `Transaction` class #854

jasonpaulos commented Jan 25, 2024 •

edited

Loading

gmalouf Feb 7, 2024 •

edited

Loading

gmalouf Feb 7, 2024 •

edited

Loading