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

fix: Use public keys for cached nonces #231

Merged
merged 5 commits into from
Nov 4, 2022
Merged

Conversation

ChaoticTempest
Copy link
Member

Moves the HashMap key from AccountId to PublicKey, since we want to associate it based on per-access-key instead of per account

Also, added debug/clone impls for the types that were added in recent PRs

@austinabell
Copy link
Contributor

Actually, what you want is a combination of the two. You can have the same public key as an access key on different accounts

Copy link
Contributor

@austinabell austinabell left a comment

Choose a reason for hiding this comment

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

my comment is opinionated and doesn't matter, feel free to ignore

workspaces/src/rpc/client.rs Outdated Show resolved Hide resolved
@@ -373,22 +373,22 @@ async fn cached_nonce(nonce: &AtomicU64, client: &Client) -> Result<(CryptoHash,
/// into contention with others.
async fn fetch_tx_nonce(
client: &Client,
account_id: AccountId,
public_key: near_crypto::PublicKey,
cache_key: &(AccountId, near_crypto::PublicKey),
Copy link
Contributor

Choose a reason for hiding this comment

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

same with this, feels like you avoid a clone if you just have this be an owned value and not take a ref outside?

Copy link
Member Author

Choose a reason for hiding this comment

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

Same with the other one with trying to optimize a little bit. But my thought was that since fetch_tx_nonce only really needs to clone on writing the nonce with key initially, it'd be better to not give it owned values everytime

The most recent commit I had should make better due of this situation though. Moved the initial creation of the cache_key outside of the retry loop, so we can just always take a reference to it, and removed the need to take ownership of the cache_key when merely just deleting the entry from the nonce map.

@ChaoticTempest
Copy link
Member Author

merging this since this also fixes the clippy issues with rust 1.65 update

@ChaoticTempest ChaoticTempest merged commit 0c02861 into main Nov 4, 2022
@ChaoticTempest ChaoticTempest deleted the fix/pubkey-for-nonces branch November 4, 2022 22:35
@frol frol mentioned this pull request Oct 4, 2023
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.

None yet

2 participants