-
Notifications
You must be signed in to change notification settings - Fork 51
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
Conversation
Actually, what you want is a combination of the two. You can have the same public key as an access key on different accounts |
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.
my comment is opinionated and doesn't matter, feel free to ignore
@@ -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), |
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.
same with this, feels like you avoid a clone if you just have this be an owned value and not take a ref outside?
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.
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.
merging this since this also fixes the clippy issues with rust 1.65 update |
Moves the HashMap key from
AccountId
toPublicKey
, since we want to associate it based on per-access-key instead of per accountAlso, added debug/clone impls for the types that were added in recent PRs