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

feat: check if coin already in cache #1549

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

hal3e
Copy link
Contributor

@hal3e hal3e commented Nov 25, 2024

Summary

Checks if any of the inputs is already cached before sending/submitting the tx.

packages/fuels-accounts/src/provider.rs Outdated Show resolved Hide resolved
digorithm
digorithm previously approved these changes Dec 2, 2024
Copy link
Contributor

@MujkicA MujkicA left a comment

Choose a reason for hiding this comment

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

IIRC the plus here is that we prevent the user from hitting the network with a tx that's bound to fail, which is great.

I have two concerns, correct me if I'm wrong:

  • We might get inconsistent results because of the time based eviction. E.g. user stalls submitting the second transaction long enough for the cache to drop the entry.
  • We don't evict when txs are dropped by the node which means we might prevent the user from executing a valid transaction at some point.

CoinTypeId::Nonce(nonce) => format!("message with nonce: `{nonce}`"),
};
Err(Error::Transaction(transaction::Reason::Validation(
format!("{msg} already in cache. Wallet address: `{addr}`, asset id: `{asset_id}`"),
Copy link
Contributor

@MujkicA MujkicA Dec 2, 2024

Choose a reason for hiding this comment

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

If the user lacks some context on how the cache or UTXO based systems work, this message might not be enough.

Saying something like:

"a transaction with this input was submitted recently, attempting to spend it again will result in an error".

should give a better idea of what the issue is.

@hal3e hal3e dismissed stale reviews from digorithm and segfault-magnet via 99f4f60 December 3, 2024 11:49
@hal3e
Copy link
Contributor Author

hal3e commented Dec 3, 2024

IIRC the plus here is that we prevent the user from hitting the network with a tx that's bound to fail, which is great.

I have two concerns, correct me if I'm wrong:

* We might get inconsistent results because of the time based eviction. E.g. user stalls submitting the second transaction long enough for the cache to drop the entry.

* We don't evict when txs are dropped by the node which means we might prevent the user from executing a valid transaction at some point.
  • If the cache drops the entry because of time based eviction then we revert to the old behavior. We send the the tx and the node will complain.
  • This can also happen with the current implementation; however, only if the tx is squeezed out.

^ Had meeting with @segfault-magnet and @MujkicA where we went through the uses cases and we came to the above conclusions.

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

Successfully merging this pull request may close these issues.

4 participants