Skip to content

Conversation

@verytactical
Copy link
Collaborator

@verytactical verytactical commented Oct 7, 2025

@thekiba
Copy link
Collaborator

thekiba commented Oct 15, 2025

Thanks for working on TEP-503 implementation. I've reviewed the changes and want to share some concerns about the current approach.

The main issue is that this introduces a breaking change by renaming ihrFee to extraFlags and changing its type. Every application currently accessing message.info.ihrFee will break after upgrading to this version. Given how foundational @ton/core is to the ecosystem, this could cause significant disruption.

I came across this gist https://gist.github.com/mr-tron/89161b6c839d7b92bdce2de6c92ee2bd that shows an approach with two distinct types: int_msg_info_ihr$00 with ihr_fee and int_msg_info_no_ihr$01 with extra_flags. This makes the distinction explicit rather than treating it as a simple rename. However, we still need to carefully think through backward compatibility since messages with the old ihr_fee field already exist on the blockchain, and we need a solution that handles those properly while introducing the new format.

What I'm uncertain about is whether to keep the existing internal type name for backward compatibility and add internal-no-ihr alongside it, or rename the existing one to internal-ihr and add internal-no-ihr so both formats are explicitly named. The first option would be less breaking, but the second might be architecturally cleaner.

Another concern is the MessageFlags type with its nested structure containing format: { type: "old" | "new", includeBody: boolean }. Since the extra_flags value determines three distinct outcomes: old format when (extra_flags & 1) = 0, new format with root when (extra_flags & 3) = 1, and new format with full body when (extra_flags & 3) = 3. Representing these three cases as simple string literals (e.g. truncate, body-root and whole-body) would be more straightforward than the current nested object structure. Also, this is specifically about bounce message behavior rather than general message flags, so the naming and structure could better reflect that.

The approach with separate message types in the union would make the structure clearer and provide better foundation for reasoning about backward compatibility. However, this is something to consider for future updates. For the current release, we should focus on finding a solution that doesn't break existing code while still enabling TEP-503 functionality. Breaking compatibility now would impact the entire ecosystem, so we need to think carefully about a transition strategy that works both short-term and long-term.

@TrueCarry
Copy link

TrueCarry commented Oct 15, 2025

I agree on the breaking change part. If we want to introduce it, we should do it in a more thought through way.

I think we should not change CommonMessageInfoInternal struct in this PR, instead we should start by adding new types introduced to blockchain(new_bounce_body) and keep it backwards compatible, so users can still update and use new types, but won't need to refactor their entire codebase.

For easier access and upgrade in the future we can make a function that accepts struct like CommonMessageInfoInternal or Coins and slice to parse and returns appropriate body

function loadNewBody(flagsInfo: CommonMessageInfoInternal | bigint | MessageBounceFlags, data: Slice): OldBody | NewBounceBody

That way we can later introduce more message formats and still keep compatibility.

I also think that we should not parse extra flags to single structure or enum MessageFlags since we would need to update it if new flags are added in the future. Instead we can keep it as coins and have simple functions that accept coins field as input and return corresponding flags

type MessageBounceFlags =  'truncate' | 'body-root' | 'whole-body'
function parseMessageBodyFlags(flags: bigint): MessageBounceFlags

ihrDisabled: boolean,
bounce: boolean,
bounced: boolean,
src?: Maybe<Address>,
Copy link

Choose a reason for hiding this comment

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

Good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants