Add withdrawable fee jettons via JettonWithdrawable - #833
Conversation
7cf3827 to
45ad703
Compare
d4c30e4 to
3b99492
Compare
3b99492 to
c9f9c64
Compare
duck-types
left a comment
There was a problem hiding this comment.
Looks good! Left some comments
|
|
||
| val taggedAsk = AskToTransfer { | ||
| queryId: ask.queryId, | ||
| jettonAmount: ask.jettonAmount, |
There was a problem hiding this comment.
This assertion should be done outside the loop. It will revert all enqueued AskToTransfer anyway
There was a problem hiding this comment.
Also, if I recal correctly, the emit also consumes value. Double check that you are not loosing balance when the received value is just enough to pay for the transfer values
There was a problem hiding this comment.
Take a look at contracts/tests/utils/sendInternalMessage.ts and how it's used to check for balance difference
There was a problem hiding this comment.
Yes, the value check could be improved.
I'll fix by using reserveToncoinsOnBalance with (1) original balance, + (2) rent due, + (3) rent reserve, or fail
| val relay = createMessage({ | ||
| bounce: BounceMode.RichBounce, | ||
| value: transfer.value, | ||
| dest: transfer.wallet, |
There was a problem hiding this comment.
Beware that this field is ment for custom Jetton implementations (see jetton specification).
custom_payload - optional custom data (which is used by either sender or receiver jetton wallet for inner logic).
Although this is pretty smart, it's a hack. We should maybe document that this library does not support Jettons that make use of customPayload, and use a more descriptive error here
There was a problem hiding this comment.
Reworked here: f5cb23c
Now using fwd payload wrap protocol (new) Jetton_ForwardPayloadWrap instead of taking over the customPayload field.
| }; | ||
|
|
||
| // Relay the transfer with the caller's value, paying forwarding fees out of it | ||
| // (SEND_MODE_REGULAR). Rich bounce returns the full original ask (with the context) on failure. |
There was a problem hiding this comment.
This should be checked at the end against totalWithdrawn
There was a problem hiding this comment.
Yeah, thanks for raising. There were gaps in logic here supporting N transfers but only a single limit with maxAmount. I've replaced it with maxAmount: map<address, coins>?, and now keep track of totalWithdrawn per wallet which should cover gaps.
| // TODO: generalized — this was added before TON token support and only supports withdrawing native TON. | ||
| // Migrate to the shared `JettonWithdrawable_WithdrawFeeTokens` (lib/funding/jetton_withdrawable.tolk) | ||
| // so the OnRamp can also withdraw accrued fee jettons, like the TokenPool does. | ||
| struct (0x7052dc75) OnRamp_WithdrawFeeTokens { |
There was a problem hiding this comment.
I don't think this can be just migrated. JettonWithdrawable_WithdrawFeeTokens doesn't support withdrawing native TON from the balance.
There was a problem hiding this comment.
I can reword, but the point is I think eventually we will want to handle fees in fee tokens (e.g., EVM ramp doesn't keep fees in ETH).
At that point we want to use JettonWithdrawable_WithdrawFeeTokens. We might still keep the native TON withdraw, used for rent.
| // | ||
| // Elements are decoded via the shared tlbe codec so both scalars and structs | ||
| // (including ones carrying ^ refs) are supported. | ||
| type Array[T any] []T |
There was a problem hiding this comment.
xssnick/tonutils-go v1.17 added support for array tags. I spent some time in July working on this branch bumping tontuils-go to latest, but never finished the work
There was a problem hiding this comment.
I will scope out a follow up for this if we already don't have a ticket
There was a problem hiding this comment.
There is a ticket I've updated here: https://smartcontract-it.atlassian.net/browse/NONEVM-4680
f5cb23c
duck-types
left a comment
There was a problem hiding this comment.
I'm not 100% sold on the opcode manual parsing
| /// Parses a `Jetton_ForwardPayloadWrap` from a `forwardPayload`, or `null` | ||
| @inline | ||
| fun Jetton_ForwardPayloadWrap.from(forwardPayload: ForwardPayloadRemainder): Jetton_ForwardPayloadWrap? { | ||
| val wrapCell = loadForwardPayloadAsCell(forwardPayload); |
There was a problem hiding this comment.
You are potentially allocating an extra cell unecessarily.
| val wrapCell = loadForwardPayloadAsCell(forwardPayload); | |
| val wrapSlice = loadForwardPayloadAsSlice(forwardPayload); |
| var s = wrapCell.beginParse(); | ||
| if (s.loadUint(32) != Jetton_ForwardPayloadWrap_OPCODE) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Why don't you use builtin decoder?
| var s = wrapCell.beginParse(); | |
| if (s.loadUint(32) != Jetton_ForwardPayloadWrap_OPCODE) { | |
| return null; | |
| } | |
| val UnpackException = 0xff | |
| try { | |
| return Jetton_ForwardPayloadWrap.fromCell(wrapCell, UnpackOptions { throwIfOpcodeDoesNotMatch: UnpackException}) | |
| } catch (e) { | |
| if (e is UnpackException) { | |
| return null; | |
| } | |
| throw e; | |
| } |
Alternativelly, you can use reflect.serializationPrefixOf to get the opcode instead of having a constant
There was a problem hiding this comment.
You can also use a lazy loading + match
|
|
||
| struct (0x2d61600c) Jetton_ForwardPayloadWrap { | ||
| /// The address the tagged operation is reported back to on failure. | ||
| initiator: address |
There was a problem hiding this comment.
Shouldn't this go inside the context, too? It's only used when the message bounces
Summary
Adds withdrawal of accrued fee jettons to the TON TokenPools via a new shared, stateless handler,
JettonWithdrawable(lib/funding/jetton_withdrawable.tolk), mirroring EVMFeeTokenHandler.WithdrawFeeTokens.This replaces the per-pool manual fee sweep with a generalized, msg-driven flow that all three pool families can share.
Highlights
Shared handler (
JettonWithdrawable)onWithdraw(msgValue, sender, msg, maxAmount, allowedRecipients) -> coinsrelays caller-suppliedAskToTransferpairs to the specified jetton wallets.msgValue >= running sum(transfer.value) + MIN_RESERVE,maxAmountbound,allowedRecipientsallowlist (throwsZeroAddressNotAllowedfor zero/none dest regardless), and a no-customPayloadguard.WithdrawContext(opcode + initiator) viacustomPayload, so a bounce is recognized and the initiator is notified with aWithdrawFailedreply (onWithdrawBounced).Pool wiring
onBouncedMessageis extended to discriminate a fee-withdrawAskToTransferbounce from the existing pool-op asks.accruedFeesledger. Fees accrue on confirmed lock settle (onLockOrBurnTransferContinueWithFeeSettlement); withdrawals are bounded by the ledger and decrement it by the total actually moved (onWithdrawFeeTokensBounded,maxAmount = st.accruedFees). A bounced withdraw ask re-credits the ledger.JettonWithdrawable_Withdrawas aTokenPool_InMessage, dispatched ononInternalMessagetoonWithdrawFeeTokensDispatch.Tests
TokenPool.withdrawFeeTokens.behavior.tsruns in all three pool specs (LockRelease,BurnMint,Lockbox): accrues-on-lock, withdraw-as-owner, withdraw-as-feeAdmin, reject-non-owner.Notes
onramp/messages.tolkTODO): migrate the OnRamp's native-TONWithdrawFeeTokensto this shared handler so it can also sweep fee jettons.