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

chore!: reorder Interface.decodeLog arguments #2874

Closed
wants to merge 8 commits into from

Conversation

nedsalk
Copy link
Contributor

@nedsalk nedsalk commented Aug 1, 2024

Summary

Having this order makes more sense.

Breaking Changes

The Interface.decodeLog arguments got reordered so that it now first accepts the logId and then the data.

// before
const abiInterface = new Interface(yourAbi);
const decodedLog = abiInterface.decodeLog(logData, logId);
// after
const decodedLog = abiInterface.decodeLog(logId, logData);

Checklist

  • I addedtests to prove my changes
  • I updated — all the necessary docs
  • I reviewed — the entire PR myself, using the GitHub UI
  • I described — all breaking changes and the Migration Guide

Copy link
Contributor

github-actions bot commented Aug 1, 2024

Coverage Report:

Lines Branches Functions Statements
79.5%(+0%) 71.8%(+0%) 77.48%(+0%) 79.64%(+0%)
Changed Files:

Coverage values did not change👌.

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.

I'm bikeshedding here so this isn't a hill I'm willing to die on as you've got 2 approvals, but I don't think we should be making breaking changes just because something seems more correct. I don't think that's enough value to justify breaking consumer apps and them having to search or deduce a resolution. And we can rewrite the API in #1795. Thoughts?

@arboleya
Copy link
Member

arboleya commented Aug 1, 2024

@danielbate You're probably right. My guess is that the typegen/coder API will inevitably break in the next big push anyway. Focusing on the new ABI spec and generated files should do the job.

@arboleya arboleya mentioned this pull request Aug 1, 2024
4 tasks
"@fuel-ts/account": patch
---

chore!: reorder `Interface.decodeLog` arguments
Copy link
Member

Choose a reason for hiding this comment

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

Approving with a comment to hold auto-merge from kicking before conversations are settled.

@nedsalk
Copy link
Contributor Author

nedsalk commented Aug 1, 2024

@danielbate @arboleya this is a change we all agree is nice, so my reasoning is that we might as well do it now.

As for #1795, I honestly believe that it won't introduce breaking changes except if we touch the design of the type-specific coders. We can have good progress by just changing how we do getCoderV1 over there and aligning it with typegen so that it's the same approach between them, and that wouldn't break it. We can then change the Coders if we want for fuels v1.

@arboleya I'm okay with closing this PR as well, it's not an important change and Interface can live on unimpeded as it is right now, so please you decide here.

@danielbate
Copy link
Contributor

@nedsalk #1795 is a v1 change. I agree we can deliver value via the points you've mentioned and not introduce breaking changes. However, the current design has been proved and battle tested, and the real value is in rethinking and rewriting both the implementation and API.

I'm pro closing this PR. I don't think the breakage justifies the value it delivers.

@petertonysmith94
Copy link
Contributor

I'd also favour closing this PR until v1 after reflection.

Thoughts @nedsalk?

@petertonysmith94
Copy link
Contributor

There appears to consensus that we'll hold off on this change till:

Closing until then.

auto-merge was automatically disabled August 12, 2024 09:15

Pull request was closed

@petertonysmith94 petertonysmith94 deleted the ns/chore/breaking-changes-in-abi-coder branch August 12, 2024 09:15
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Issue is a chore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants