fix!: remove offset from `decodeLog` and `decodeFunctionResult` methods #2308

DZakh · 2024-05-15T08:17:53Z

It feels like public decode functions should return the decoded value itself. Not a tuple with an internal offset.

CLAassistant · 2024-05-15T08:17:59Z

All committers have signed the CLA.

Torres-ssf · 2024-05-15T15:07:13Z

Hey @DZakh, thanks for taking an interest in helping us to improve the TS SDK, we appreciate it 🙏.

Could you please elaborate on why you believe these changes are necessary? Understanding your reasoning will help us assess the impact and benefits of this modification.

DZakh · 2024-05-20T10:11:03Z

I don't have a strong feeling about the change, and it's not required for my project.
It was mostly done after the following reasoning:

The offset is not needed as a result of a high-level function such as decodeLog
The decodeFunctionData doesn't return an offset

If you say it's intentional, it would be nice to change the return type from any to a tuple with an offset as a second argument. Because it took me some time to understand what was wrong with the decoded data.

danielbate

Hey @DZakh, after investigation I do agree with you and I think it was oversight that these entry points for function and log value decoding would include the offset. Most SDK consumers will utilise the TransactionResponse or FunctionInvocationResult classes that we return on sendTransaction() or call() respectively that internally will pick off the decoded values rather than returning the tuple.

To get this over the line, it'd be great if you can add a changeset (run pnpm changeset on the monorepo and it would also be great to get some tests so this is not missed in the future, I would reccomend adding them to packages/abi-coder/test/Interface.test.ts. Let me know if I can help with either of these.

DZakh · 2024-05-21T13:34:36Z

Hey @DZakh, after investigation I do agree with you and I think it was oversight that these entry points for function and log value decoding would include the offset. Most SDK consumers will utilise the TransactionResponse or FunctionInvocationResult classes that we return on sendTransaction() or call() that internally will pick off the decoded values rather than returning the tuple.

To get this over the line, it'd be great if you can add a changeset (run pnpm changeset on the monorepo and it would also be great to get some tests so this is not missed in the future, I would reccomend adding them to packages/abi-coder/test/Interface.test.ts. Let me know if I can help with either of these.

I'll try to find some time during the week 👌

DZakh

Could you take a look, please

packages/abi-coder/src/Interface.ts

packages/abi-coder/test/Interface.test.ts

DZakh · 2024-05-23T14:58:27Z

packages/abi-coder/src/Interface.ts

-      `function ${nameOrSignatureOrSelector} not found: ${JSON.stringify(fn)}.`
+      `Function ${nameOrSignatureOrSelector} not found.`


fn is undefined here

DZakh · 2024-05-23T15:11:48Z

I'm done with my changes. Ready for re-review

nedsalk · 2024-05-24T08:51:24Z

packages/abi-coder/test/Interface.test.ts

+
+  describe('decodeLog', () => {
+    it('should return decoded log by id', () => {
+      const data = exhaustiveExamplesInterface.decodeLog('0x01000000000000000000000000000020', '0');


The logId passed here should be "8500535089865083573" when the sway program is built with forc 0.59.0.

Also, passing in Uint8Array.from([1, 0, 0, 0, 32]) here works.

nedsalk · 2024-05-24T08:59:39Z

packages/abi-coder/test/Interface.test.ts

+    it('should throw an error when log does not exist', () => {
+      expect(() =>
+        exhaustiveExamplesInterface.decodeLog('0x01000000000000000000000000000020', '1')
+      ).toThrowError(`Log type with logId '1' doesn't exist in the ABI.`);


You should use the expectToThrowFuelError utility here (example).

nedsalk · 2024-05-24T08:59:48Z

packages/abi-coder/test/Interface.test.ts

+          'doesnt_exist',
+          '0x01000000000000000000000000000020'
+        );
+      }).toThrowError(/^Function doesnt_exist not found\.$/);


You should use the expectToThrowFuelError utility here (example).

nedsalk · 2024-05-24T09:05:14Z

.changeset/perfect-kings-camp.md

+"@fuel-ts/abi-coder": minor
+---
+
+Changed decodeLog and decodeFunctionResult return types to have only decoded value without an offset


Suggested change

Changed decodeLog and decodeFunctionResult return types to have only decoded value without an offset

fix!: don't return offset from public decode functions

We have some CI logic that mandates that this text is equal to the pr title.

nedsalk · 2024-05-24T09:05:46Z

@DZakh Thanks for the work! I agree that the offset shouldn't be returned.

petertonysmith94 · 2024-05-27T07:24:08Z

.changeset/perfect-kings-camp.md

+"@fuel-ts/abi-coder": minor
+---
+
+fix: Changed decodeLog and decodeFunctionResult return types to have only decoded value without an offset


Suggested change

fix: Changed decodeLog and decodeFunctionResult return types to have only decoded value without an offset

fix!: changed `decodeLog` and `decodeFunctionResult` return types to have only decoded value without an offset

I believe this is a breaking change, as @nedsalk mentioned, this will need to be in sync with the title 😄

DZakh · 2024-06-03T08:28:32Z

I've just returned from a one-week vacation. I didn't touch my laptop at all, and now I'm a little bit lost on what's left for the PR 😅 Could somebody summarize it for me, please?

nedsalk · 2024-06-06T16:40:10Z

@DZakh Thanks for the work here, I've also assigned myself to the PR and will take this over the line.

DZakh · 2024-06-07T08:10:41Z

@DZakh Thanks for the work here, I've also assigned myself to the PR and will take this over the line.

Thanks

nedsalk · 2024-06-13T13:00:13Z

@DZakh I don't have the rights to push to your fork. You can find my changes on this branch. Please merge them into your fork so that we can continue with the review.

DZakh · 2024-06-13T14:22:41Z

@nedsalk, I don't mind if we continue with your PR. Let's just close this one and reference yours.

nedsalk · 2024-06-13T15:24:10Z

Superseded by #2514

DZakh requested review from digorithm, arboleya, Torres-ssf, Dhaiwat10, danielbate, nedsalk, petertonysmith94 and maschad as code owners May 15, 2024 08:17

Torres-ssf assigned DZakh May 15, 2024

danielbate reviewed May 21, 2024

View reviewed changes

fix: Don't return offset from public decode functions

99aa766

DZakh force-pushed the fix/remove-offset-from-decode-result branch from a32515f to 99aa766 Compare May 23, 2024 13:51

DZakh added 2 commits May 23, 2024 18:31

Add tests for decodeLog

e5ccfa4

Add .toString() to search for logs

dbe1564

DZakh commented May 23, 2024

View reviewed changes

packages/abi-coder/src/Interface.ts Outdated Show resolved Hide resolved

packages/abi-coder/test/Interface.test.ts Show resolved Hide resolved

DZakh requested a review from danielbate May 23, 2024 14:40

Add decodeFunctionResult tests

c71bd01

DZakh commented May 23, 2024

View reviewed changes

Add changeset

46cc00b

nedsalk reviewed May 24, 2024

View reviewed changes

nedsalk changed the title ~~fix: Don't return offset from public decode functions~~ fix!: don't return offset from public decode functions May 24, 2024

nedsalk reviewed May 24, 2024

View reviewed changes

DZakh changed the title ~~fix!: don't return offset from public decode functions~~ fix!: Changed decodeLog and decodeFunctionResult return types to have only decoded value without an offset May 24, 2024

DZakh changed the title ~~fix!: Changed decodeLog and decodeFunctionResult return types to have only decoded value without an offset~~ fix: Changed decodeLog and decodeFunctionResult return types to have only decoded value without an offset May 24, 2024

DZakh added 2 commits May 24, 2024 19:53

Update tests

a6f1494

Fix changelog

7799643

petertonysmith94 reviewed May 27, 2024

View reviewed changes

arboleya changed the title ~~fix: Changed decodeLog and decodeFunctionResult return types to have only decoded value without an offset~~ fix: remove offset from decodeLog and decodeFunctionResult methods May 30, 2024

This comment was marked as outdated.

Sign in to view

arboleya assigned nedsalk Jun 6, 2024

danielbate mentioned this pull request Jun 7, 2024

v1 ABI Coder migration cleanup #2462

Closed

arboleya added this to the 0.x post-launch milestone Jun 12, 2024

nedsalk changed the title ~~fix: remove offset from decodeLog and decodeFunctionResult methods~~ fix!: remove offset from decodeLog and decodeFunctionResult methods Jun 13, 2024

nedsalk added the feat Issue is a feature label Jun 13, 2024

nedsalk mentioned this pull request Jun 13, 2024

feat!: remove offset from decodeLog and decodeFunctionResult methods #2514

Closed

nedsalk closed this Jun 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix!: remove offset from `decodeLog` and `decodeFunctionResult` methods #2308

fix!: remove offset from `decodeLog` and `decodeFunctionResult` methods #2308

DZakh commented May 15, 2024

CLAassistant commented May 15, 2024 •

edited

Loading

Torres-ssf commented May 15, 2024

DZakh commented May 20, 2024

danielbate left a comment •

edited

Loading

DZakh commented May 21, 2024

DZakh left a comment

DZakh May 23, 2024

DZakh commented May 23, 2024

nedsalk May 24, 2024 •

edited

Loading

nedsalk May 24, 2024

nedsalk May 24, 2024

nedsalk May 24, 2024

nedsalk commented May 24, 2024

petertonysmith94 May 27, 2024 •

edited

Loading

DZakh commented Jun 3, 2024

This comment was marked as outdated.

nedsalk commented Jun 6, 2024

DZakh commented Jun 7, 2024

nedsalk commented Jun 13, 2024

DZakh commented Jun 13, 2024

nedsalk commented Jun 13, 2024

		`function ${nameOrSignatureOrSelector} not found: ${JSON.stringify(fn)}.`
		`Function ${nameOrSignatureOrSelector} not found.`

	Changed decodeLog and decodeFunctionResult return types to have only decoded value without an offset
	fix!: don't return offset from public decode functions

	fix: Changed decodeLog and decodeFunctionResult return types to have only decoded value without an offset
	fix!: changed `decodeLog` and `decodeFunctionResult` return types to have only decoded value without an offset

fix!: remove offset from decodeLog and decodeFunctionResult methods #2308

fix!: remove offset from decodeLog and decodeFunctionResult methods #2308

Conversation

DZakh commented May 15, 2024

CLAassistant commented May 15, 2024 • edited Loading

Torres-ssf commented May 15, 2024

DZakh commented May 20, 2024

danielbate left a comment • edited Loading

Choose a reason for hiding this comment

DZakh commented May 21, 2024

DZakh left a comment

Choose a reason for hiding this comment

DZakh May 23, 2024

Choose a reason for hiding this comment

DZakh commented May 23, 2024

nedsalk May 24, 2024 • edited Loading

Choose a reason for hiding this comment

nedsalk May 24, 2024

Choose a reason for hiding this comment

nedsalk May 24, 2024

Choose a reason for hiding this comment

nedsalk May 24, 2024

Choose a reason for hiding this comment

nedsalk commented May 24, 2024

petertonysmith94 May 27, 2024 • edited Loading

Choose a reason for hiding this comment

DZakh commented Jun 3, 2024

This comment was marked as outdated.

nedsalk commented Jun 6, 2024

DZakh commented Jun 7, 2024

nedsalk commented Jun 13, 2024

DZakh commented Jun 13, 2024

nedsalk commented Jun 13, 2024

fix!: remove offset from `decodeLog` and `decodeFunctionResult` methods #2308

fix!: remove offset from `decodeLog` and `decodeFunctionResult` methods #2308

CLAassistant commented May 15, 2024 •

edited

Loading

danielbate left a comment •

edited

Loading

nedsalk May 24, 2024 •

edited

Loading

petertonysmith94 May 27, 2024 •

edited

Loading