Adds TransferSharesToManyV1 to QUTIL#803
Adds TransferSharesToManyV1 to QUTIL#803double-k-3033 wants to merge 5 commits intoqubic:developfrom
Conversation
Release v1.282.0
Release v1.282.0 Add QBridge contract (Vottun) Add Oracle Documentation Fix bug on loading execution fee after seamless epoch change Fix problem during loading of assets from snapshot Adjust TICK_DURATION_FOR_ALLOCATION_MS and TRANSACTION_SPARSENESS Increase ADDITION_SOLUTION_THRESHOLD_DEFAULT to 74800
philippwerner
left a comment
philippwerner
left a comment
There was a problem hiding this comment.
Thanks for the contribution. Overall, it looks good to me. Just the following comments:
Why did you change the DebugInformationFormat in the platform libraries? Is there any problem with the "ProgramDatabase" setting that is fixed by changing the value to "OldStyle"? If there isn't a good reason, please undo the changes in the *.vcxproj files.
If management rights are transferred to QUTIL, it also must offer a way to transfer the rights back to other contracts. So please add a procedure TransferShareManagementRights for that purpose to QUTIL.
|
I'm sorry. An error occurred during the build process, so I had no choice but to change it that way. I forgot that I shouldn't push the related files when I push. |
double-k-3033
force-pushed
the
feature/qutil
branch
from
c794718 to
e8f0f14
Compare
double-k-3033
force-pushed
the
feature/qutil
branch
from
e8f0f14 to
9ce0779
Compare
double-k-3033
force-pushed
the
feature/qutil
branch
from
51ca19e to
e20242b
Compare
philippwerner
left a comment
philippwerner
left a comment
There was a problem hiding this comment.
Thanks for your contribution. Please address the following comments.
| * @param list of 24 amounts (192 bytes): 8 bytes(sint64) for each amount, leave 0 for unused slots | ||
| * @return returnCode (0 means success) | ||
| */ | ||
| PUBLIC_PROCEDURE_WITH_LOCALS(TransferShareToManyV1) |
There was a problem hiding this comment.
Please rename to "TransferSharesToManyV1" (plural shares)
| LOG_INFO(locals.logger); | ||
|
|
||
| // Validate invocation fee | ||
| if (qpi.invocationReward() < state.get().smt1InvocationFee) |
There was a problem hiding this comment.
The smt1InvocationFee is too low. After some discussion with JOETOM and Kavatak, we came to the conclusion that a good solution will be to:
- query the current transfer fee of QX = tf with
CALL_OTHER_CONTRACT_FUNCTION(QX, Fees, ..., ...), - compute the fee as
tstmInvocationFee = 25 * tf, - check if
qpi.invocationReward()is sufficient (as done here, but with tstmInvocationFee), - transfer the shares,
- transfer
24 * tfto QX, - burn
1 * tf.
| Asset asset; | ||
| sint64 numberOfShares; | ||
| uint32 newManagingContractIndex; | ||
| sint64 offeredTransferFee; |
There was a problem hiding this comment.
I don't see why we need offeredTransferFee here. Just use the invocation reward as the offered fee for simplicity. For proper handling of the case that the offeredTransferFee is greater than the requested fee, you should store the return value of qpi.releaseShares(), which is the fee actually paid for releasing if the value is >= 0.
| } | ||
|
|
||
| // Release management rights from QUTIL to destination contract | ||
| if (qpi.releaseShares( |
There was a problem hiding this comment.
The function qpi.releaseShares() internally does all the checks that you have done above, so you may remove those.
| // Safe refund: anything above the offered fee is definitely not needed | ||
| if (qpi.invocationReward() > input.offeredTransferFee) | ||
| { | ||
| qpi.transfer(qpi.invocator(), qpi.invocationReward() - input.offeredTransferFee); |
There was a problem hiding this comment.
Here you should refund the difference between the invocation reward and the fee that was actually paid (returned by qpi.releaseShares()). Or you should burn the difference. At the moment it will just remain in the balance of the contract.
5 participants
No description provided.