Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed

Conversation

DZakh
Copy link

@DZakh DZakh commented May 15, 2024

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

@CLAassistant
Copy link

CLAassistant commented May 15, 2024

CLA assistant check
All committers have signed the CLA.

@Torres-ssf
Copy link
Contributor

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
Copy link
Author

DZakh commented May 20, 2024

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.

Copy link
Contributor

@danielbate danielbate left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Author

DZakh commented May 21, 2024

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 DZakh force-pushed the fix/remove-offset-from-decode-result branch from a32515f to 99aa766 Compare May 23, 2024 13:51
Copy link
Author

@DZakh DZakh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you take a look, please

packages/abi-coder/src/Interface.ts Outdated Show resolved Hide resolved
packages/abi-coder/test/Interface.test.ts Show resolved Hide resolved
@DZakh DZakh requested a review from danielbate May 23, 2024 14:40
`function ${nameOrSignatureOrSelector} not found: ${JSON.stringify(fn)}.`
`Function ${nameOrSignatureOrSelector} not found.`
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fn is undefined here

@DZakh
Copy link
Author

DZakh commented May 23, 2024

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


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

@nedsalk nedsalk May 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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.`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should use the expectToThrowFuelError utility here (example).

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should use the expectToThrowFuelError utility here (example).

@nedsalk 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
"@fuel-ts/abi-coder": minor
---

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

nedsalk commented May 24, 2024

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

@DZakh 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 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
"@fuel-ts/abi-coder": minor
---

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

@petertonysmith94 petertonysmith94 May 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😄

@arboleya 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
@DZakh
Copy link
Author

DZakh commented Jun 3, 2024

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?

@danielbate

This comment was marked as outdated.

@nedsalk
Copy link
Contributor

nedsalk commented Jun 6, 2024

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

@DZakh
Copy link
Author

DZakh commented Jun 7, 2024

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

Thanks

@arboleya arboleya added this to the 0.x post-launch milestone Jun 12, 2024
@nedsalk nedsalk changed the title fix: remove offset from decodeLog and decodeFunctionResult methods fix!: remove offset from decodeLog and decodeFunctionResult methods Jun 13, 2024
@nedsalk nedsalk added the feat Issue is a feature label Jun 13, 2024
@nedsalk
Copy link
Contributor

nedsalk commented Jun 13, 2024

@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
Copy link
Author

DZakh commented Jun 13, 2024

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

@nedsalk
Copy link
Contributor

nedsalk commented Jun 13, 2024

Superseded by #2514

@nedsalk nedsalk closed this Jun 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat Issue is a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants