Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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(tpu-v2): fix tpu-v2 wait for payment spend and extract secret #2261
base: dev
Are you sure you want to change the base?
fix(tpu-v2): fix tpu-v2 wait for payment spend and extract secret #2261
Changes from 10 commits
9d7f476
de3a970
ea5475b
934a2f0
ca0ee56
9449cca
2e65abf
43a1baf
c700b59
8d5ed46
707946a
1e7a197
fb383fb
9ec0bab
6c8b2c1
6dcb1c3
f817f5c
ddac3c8
f437dde
02736e4
a08b500
24cb4c3
5f666f3
47f904c
2056f9b
0b3ab9a
cf76cab
c1a5063
43c2d94
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we advance
from_block
if the event wasn't found?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't change blocks range, It introduces risks to skip necessary block.
Receiving empty events list does not necessarily indicate that there are no events, network latency can cause delays in the propagation and indexing of event logs even after a transaction is mined.
After a transaction is mined, the logs related to it need to be extracted and made available for querying. This process is not instantaneous.
Also we dont know all the nuances and differences of all blockchains. It is much safer to keep block range starting from swap start block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UPD: l suggest to add
if events.is_empty
check to continue loop without moving forwardUPD2: added is empty check 8d5ed46
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will this ever be None? will this be a recoverable state then? otherwise we can terminate early.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you talking about
event.transaction_hash
? This type is from a dependency, we should handle it as it is. Thetransaction_hash
could beNone
if the log is emitted by a transaction in a pending state. Once the transaction is included in a mined block, the value should be Some.For TPU V2 we aim to have automatic recover process, if
find_taker_payment_spend_tx
return error then refund process will be startedkomodo-defi-framework/mm2src/mm2_main/src/lp_swap/taker_swap_v2.rs
Lines 1698 to 1716 in c700b59
Could clarify what do you mean by termination? You want to return error and break loop?
We should try to find transaction in the loop until time is out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aren't we getting events till the current mined block? so this tx shouldn't be pending?
I just meant we can not do nothing about the fact that tx hash is none.
nothing to do with swap recovery.
how I was thinking (which might be wrong) is that some event types don't have a tx hash which means we supplied a bad event id from the beginning meaning that we can't proceed further. this might not be the case though, gotta read more about this, excuse my eth illiteracy 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, you are right as we are using
from
andto
block filters foreth_getLogs
API, tx should be confirmedI think you are confused a bit. Events/logs themselves don't necessarily contain the transaction hash. Instead, the transaction hash is associated with the transaction that emitted the event. So Log type from web3 just contains info about tx which emitted this log.
Note about event and log words. event in Solidity is a way for a contract to emit logs during the execution of a function.
So they are close words, just events refer to the Solidity construct used in the smart contract to emit logs, while logs refer to the actual data that is recorded in the blockchain when events are emitted.
So using from to block range we are looking for Log which was emitted by spend taker payment transaction.
As for empty tx hash I would like to refer to previous comment #2261 (comment) empty event list or none transaction_hash are not 100% that there is no tx which we are looking for, it could be just blockchain indexation delays or other reasons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i will read further regarding the truncated (not necessarily empty? right?) event list issue, any resourced regarding the indexaction delay issues and such would be so helpful!
regarding an event not having a
transaction_hash
thought, how would we successfully get the event which has None for thetransaction_hash
and then try again and all of a sudden we get a Sometransaction_hash
! is that possible? are these event logs mutable?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say that chain reorganization, network latency, node synchronization lag issues could cause missing information issues. These problems usually temporarily, as I understand. You should wait for more confirmed blocks also try to fetch the info from different nodes.
Logs are not mutable. Also they are tied to transactions. When a transaction calls a smart contract function that emits an event, this event generates log, which is permanently recorded in the blockchain.
But there’s a nuance. Lest check doc. According to the documentation https://www.chainnodes.org/docs/ethereum/eth_getLogs, a log from a pending transaction might lack a transaction hash, but when the transaction is confirmed, the log should include it.
Therefore, ideally, when we request a list of logs using the
events_from_block
function withfrom_block
andto_block
filters, it should return only logs from confirmed blocks, which means confirmed transactions. In this case,event.transaction_hash
should ideally always beSome
.komodo-defi-framework/mm2src/coins/eth/eth_swap_v2/eth_taker_swap_v2.rs
Lines 530 to 531 in 8d5ed46
We dont need to change from_block. However, if Log has
transaction_hash:None
, it doesn't mean Log doesn't have transaction (actually its not correct by itself, as logs are not owners of txs), it means smth went wrong as eth_getLogs API with fromBlock and toBlock filters will use confirmed blocks.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some additional info https://docs.alchemy.com/docs/deep-dive-into-eth_getlogs#what-are-logs-or-events
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
im missing u between the lines here so let me repeat that to you and see if i got it correctly.
transaction_hash
MUST always beSome
eventually, if it wasNone
this is a temporary reporting/mining/etc... thing.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, in the context of not using "pending" tag in "eth_getLogs" API, we expect
transaction_hash
always beSome
.The best we can do is to repeat loop cycle until time is out, if None occurred.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now that we have the tx_hash, we shouldn't keep looping right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm yes
once we got
Some(tx_hash)
it means event we found had necessary swap id.But if we got None or error in next step after
Some(tx_hash)
, we dont have to repeat the whole process from the beginning.Next loop cycle we can start right from requesting transaction from
tx_hash
. I will work on it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 6c8b2c1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should abort if we got error as this can lead to a potential infinite loop.self.transaction
return error only if the tx data can't be deserialize which means bad tx data so we should breakThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no infinite loop, we have
if now > wait_until
checkno, it has
try_rpc_send
function, it could returnweb3::Error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok.. then you should still handle the error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you confused
transaction
andsigned_tx_from_web3_tx
functionskomodo-defi-framework/mm2src/coins/eth/eth_swap_v2/eth_taker_swap_v2.rs
Lines 528 to 541 in f437dde
transaction
uses web3 calls.signed_tx_from_web3_tx
is one which doesnt have web3 requests and tries to build ethcore transaction from web3 transaction. If it fails then it is useless to repeat it, we can return error instantly and we do it