modified the AssembledTransaction and it's tests #1075

PoulavBhowmick03 · 2024-10-02T06:32:01Z

Fixes #1059

Description:

Corrected isReadOnly Method Invocation:
- Issue: Previously, the isReadOnly method was incorrectly invoked on an instance of ScSpecFunctionV0, which does not possess this method.
- Solution: Updated the invocation to use the Spec instance (this.spec.isReadOnly(method)) ensuring proper type safety and functionality.
Enhanced AssembledTransaction Class:
- Feature Addition: Modified the sign method within the AssembledTransaction class to fail fast if no publicKey is provided.
- Improved UX: Instead of using a generic error message or defaulting to a hardcoded NULL_ACCOUNT, the sign method now throws a descriptive error:
```
"Public key not provided. Have you forgotten to set the `publicKey` in AssembledTransactionOptions?"
```
- Benefit: This change ensures developers receive clear guidance when they omit the publicKey, enhancing the developer experience and preventing unintended transaction behaviors.
Updated Test Files to Reflect Changes:
- assembled_transaction_test.js:
  - Imported and instantiated the Spec class with the contract's specification entries.
  - Updated the buildFootprintRestoreTransaction method to pass the Spec instance.
  - Changed account instantiation from StellarSdk.Account to xdr.Account to align with expected types.
  - Ensured transaction fees are passed as strings to match method signatures.
- test-swap.js:
  - Imported and instantiated the Spec class with the appropriate contract specifications.
  - Updated transaction building and signing processes to utilize the Spec instance.
  - Adjusted error assertions to correctly reference the updated error types.
  - Ensured consistent account types and fee formatting in transaction methods.
Enhanced Spec Class (spec.ts):
- Implemented the isReadOnly method to accurately determine if a contract function is read-only based on naming conventions or metadata.
- Added comprehensive documentation for better maintainability and clarity.
General Improvements:
- Type Safety: Ensured all method invocations and parameter types align with TypeScript standards, preventing potential runtime errors.
- Descriptive Error Messages: Enhanced error messages to be more actionable, facilitating easier debugging.
- Comprehensive Testing: Updated test cases to cover both read-only and write transaction scenarios, ensuring robust test coverage.

Benefits:

Resolved TypeScript Error: Eliminates the Property 'isReadOnly' does not exist on type 'ScSpecFunctionV0'.ts(2339) error by correctly invoking isReadOnly on the Spec instance.
Improved Developer Experience: Clear and descriptive error messages guide developers to provide necessary configurations, reducing frustration and potential bugs.
Enhanced Code Quality: Strengthens the codebase through proper type handling, error management, and comprehensive testing.
Reliable Transaction Handling: Ensures transactions behave as expected, with appropriate checks in place to prevent unintended operations.

Testing:

Unit Tests: All existing unit tests pass, and new tests cover the scenarios where publicKey is omitted, ensuring the sign method behaves correctly.
Integration Tests: Verified that transactions requiring publicKey fail gracefully when omitted and succeed when provided.
Error Handling: Confirmed that descriptive errors are thrown, aiding in quicker debugging and issue resolution.

src/contract/assembled_transaction.ts

src/contract/client.ts

src/contract/spec.ts

PoulavBhowmick03 · 2024-10-08T05:42:04Z

@Shaptic PTAL

chadoh

I didn't finish reviewing the tests, but there's a lot to fix up here. I can come back and review again after your next pass.

Sorry you didn't have more context for how Stellar works and the real need here before you got started the first time 😞

src/contract/assembled_transaction.ts

src/contract/client.ts

src/contract/spec.ts

src/contract/types.ts

test/e2e/src/test-swap.js

PoulavBhowmick03 · 2024-10-14T11:49:07Z

@chadoh PTAL, sorry i was going through some rough time, my tg and metamask wallet got hacked, lost around 200 dollars... So it took a little time
Do have a look at the changes

chadoh · 2024-10-21T15:37:17Z

src/contract/assembled_transaction.ts

+    // Ensure publicKey is provided for non-read calls
+    if (!this.options.publicKey) {
+      throw new Error(
+        "Public key not provided. Have you forgotten to set the `publicKey` in AssembledTransactionOptions?"


It looks like this is the only needed change here. Everything except this block seems either like anti-linting or outright error. Maybe there's a test of this specific logic that should stay; I again didn't get that far.

Can you please force-push a single commit to this branch with only these specific changes? We do not want to merge anything else.

I see you pushed right to your master branch so it might be a little more complicated and error-prone, but here's a way you can do it:

log --pretty=oneline -n 10 --graph – this should show you the commit, eight commits ago, where you branched off what was then master on the upstream remote (not sure how your local remotes are named). Copy that commit hash.

git reset --hard [pasted commit hash] – this will reset all local changes to your branch

git pull upstream master – may as well update to the tip of the upstream branch to avoid as many conflicts as possible. Again, not sure what your local remotes are called. You can use git remote -v to see all remotes and make sure you have the name of the stellar/js-stellar-sdk one, and slot it in where I put the upstream.

Now you can copy-paste specific changes from this PR back into your local changes and commit them with a sensible commit message.

git push --force origin master – one more time, not sure what your remotes are called. Make sure to use whatever name you gave your remote, rather than origin.

…ided

PoulavBhowmick03 · 2024-10-21T16:10:10Z

@chadoh do take a look now

chadoh · 2024-10-21T17:14:46Z

src/contract/assembled_transaction.ts

+    if (!options.publicKey) {
+      throw new Error("Public key not provided. Have you forgotten to set the `publicKey` in AssembledTransactionOptions?");


Still a lot of anti-lint noise and erroneous code like this

…ided

PoulavBhowmick03 · 2024-10-21T17:21:11Z

@chadoh I have run the yarn fmt and yarn build and then pushed. are there any more blockages?

chadoh

Here I've marked eight areas that need improvement; just about the limit for what will show without GitHub collapsing it. Please don't make me go through the rest of the PR and comment on every single line. Please git reset to the point you branched of master and create ONE new commit, as I previously advised, and ONLY copy-paste in the correct changes. Thank you.

chadoh · 2024-10-21T17:22:42Z

src/contract/assembled_transaction.ts

-    if(!this.built) throw new Error(
-        "Transaction has not yet been simulated; " +
-        "call `AssembledTransaction.simulate` first.",
-      );
+    if (!this.built) throw new Error(
+      "Transaction has not yet been simulated; " +
+      "call `AssembledTransaction.simulate` first.",
+    );


None of these changes should be included

chadoh · 2024-10-21T17:22:49Z

src/contract/assembled_transaction.ts

-      { ...options,
+      {
+        ...options,


chadoh · 2024-10-21T17:22:56Z

src/contract/assembled_transaction.ts

-     );
+    );


chadoh · 2024-10-21T17:23:15Z

src/contract/assembled_transaction.ts

+    if (!options.publicKey) {
+      throw new Error("Public key not provided");
+    }


See previous comments about this. The logic is wrong. We do not want this.

chadoh · 2024-10-21T17:23:32Z

src/contract/assembled_transaction.ts

+  // AssembledTransaction.ts
+


This is noise

chadoh · 2024-10-21T17:23:40Z

src/contract/assembled_transaction.ts

+    if (!options.publicKey) {
+      throw new Error("Public key not provided. Have you forgotten to set the `publicKey` in AssembledTransactionOptions?");
+    }
+


This is wrong

chadoh · 2024-10-21T17:23:46Z

src/contract/assembled_transaction.ts

-    const account = await getAccount(
-      options,
-      tx.server
-    );
+    const account = await getAccount(options, tx.server);


This is wrong

chadoh · 2024-10-21T17:23:52Z

src/contract/assembled_transaction.ts

@@ -464,6 +471,7 @@ export class AssembledTransaction<T> {
    return tx;
  }

+


This is noise

modified the AssembledTransaction and it's tests

8e8b7ae

Shaptic reviewed Oct 7, 2024

View reviewed changes

Merge branch 'master' into master

dbf1f58

chadoh suggested changes Oct 9, 2024

View reviewed changes

chadoh suggested changes Oct 21, 2024

View reviewed changes

fixed: AssembledTransaction's sign failing when no public key is prov…

0c556e7

…ided

PoulavBhowmick03 force-pushed the master branch from 1dc5f97 to 0c556e7 Compare October 21, 2024 16:09

chadoh suggested changes Oct 21, 2024

View reviewed changes

fixed: AssembledTransaction's sign failing when no public key is prov…

016535b

…ided

chadoh suggested changes Oct 21, 2024

View reviewed changes

PoulavBhowmick03 closed this Nov 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

modified the AssembledTransaction and it's tests #1075

modified the AssembledTransaction and it's tests #1075

PoulavBhowmick03 commented Oct 2, 2024

PoulavBhowmick03 commented Oct 8, 2024

chadoh left a comment

PoulavBhowmick03 commented Oct 14, 2024

chadoh Oct 21, 2024

PoulavBhowmick03 commented Oct 21, 2024

chadoh Oct 21, 2024

PoulavBhowmick03 commented Oct 21, 2024

chadoh left a comment

chadoh Oct 21, 2024

chadoh Oct 21, 2024

chadoh Oct 21, 2024

chadoh Oct 21, 2024

chadoh Oct 21, 2024

chadoh Oct 21, 2024

chadoh Oct 21, 2024

chadoh Oct 21, 2024

		if (!options.publicKey) {
		throw new Error("Public key not provided. Have you forgotten to set the `publicKey` in AssembledTransactionOptions?");

		@@ -464,6 +471,7 @@ export class AssembledTransaction<T> {
		return tx;
		}

modified the AssembledTransaction and it's tests #1075

modified the AssembledTransaction and it's tests #1075

Conversation

PoulavBhowmick03 commented Oct 2, 2024

Description:

PoulavBhowmick03 commented Oct 8, 2024

chadoh left a comment

Choose a reason for hiding this comment

PoulavBhowmick03 commented Oct 14, 2024

Choose a reason for hiding this comment

PoulavBhowmick03 commented Oct 21, 2024

Choose a reason for hiding this comment

PoulavBhowmick03 commented Oct 21, 2024

chadoh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment