soft delete nonce in s_consumers so that request IDs do not repeat #12405
Conversation
jinhoonbang
requested review from
kidambisrinivas,
leeyikjiun,
vreff and
ibrajer
and removed request for
a team
|
I see that you haven't updated any README files. Would it make sense to do so? |
| mapping(uint256 => uint64) storage nonces = s_consumers[consumer]; | ||
| if (nonces[subId] != 0) { | ||
| ConsumerConfig storage consumerConfig = s_consumers[consumer][subId]; | ||
| if (consumerConfig.active) { |
There was a problem hiding this comment.
Should we move this check before if (consumers.length == MAX_CONSUMERS) {, since attempting to add same consumer again should be an idempotent operation instead of reverting
There was a problem hiding this comment.
Yeah that makes sense technically. Can change it
| // Idempotence - do nothing if already added. | ||
| // Ensures uniqueness in s_subscriptions[subId].consumers. | ||
| return; | ||
| } | ||
| // Initialize the nonce to 1, indicating the consumer is allocated. | ||
| nonces[subId] = 1; | ||
| // consumerConfig.nonce is 0 if the consumer had never sent a request to this subscription |
There was a problem hiding this comment.
Should we consider starting nonce at 1, to maintain parity with previous implementation
There was a problem hiding this comment.
I think 0 makes more sense because previously 0 indicated inactive consumer but now, we have a active boolean flag.
Also, we increment nonce before generating the first requestID here. So if we start the nonce at 1 here, first requestID will be generated with nonce 2
There was a problem hiding this comment.
Also, as long as the boolean is true to start the next SSTORE will not be cold.
| @@ -49,7 +49,7 @@ var rcTemplate = `{ | |||
| callbackGasLimit: %d, | |||
| numWords: %d, | |||
| sender: %s, | |||
| nativePayment: %t | |||
| nativePayment: %s | |||
There was a problem hiding this comment.
not sure why we had %t here initially. makes more sense to print hex-encoded string of the extraArgs instead of the type. this should be renamed to extraArgs actually.
There was a problem hiding this comment.
Isn't lowercase %t boolean in Go? Or am I mixing that up?
There was a problem hiding this comment.
oh right. yeah it was trying to display bytes as bool
|
jinhoonbang
requested review from
kidambisrinivas,
vreff,
leeyikjiun and
ibrajer
|
nit: maybe if it's in merge queue we can drop the WIP from the PR title? |
jinhoonbang
changed the title
…IP (#12405) * soft delete nonce in s_consumers so that request IDs do not repeat. WIP * forge unit tests passing except for exact billing amount * add forge tests and optimize gas * regenerate wrappers * remove comment * address comments * fix test * add changeset
…IP (#12405) * soft delete nonce in s_consumers so that request IDs do not repeat. WIP * forge unit tests passing except for exact billing amount * add forge tests and optimize gas * regenerate wrappers * remove comment * address comments * fix test * add changeset
…IP (#12405) * soft delete nonce in s_consumers so that request IDs do not repeat. WIP * forge unit tests passing except for exact billing amount * add forge tests and optimize gas * regenerate wrappers * remove comment * address comments * fix test * add changeset
…IP (#12405) * soft delete nonce in s_consumers so that request IDs do not repeat. WIP * forge unit tests passing except for exact billing amount * add forge tests and optimize gas * regenerate wrappers * remove comment * address comments * fix test * add changeset
…IP (#12405) * soft delete nonce in s_consumers so that request IDs do not repeat. WIP * forge unit tests passing except for exact billing amount * add forge tests and optimize gas * regenerate wrappers * remove comment * address comments * fix test * add changeset
soft delete nonce in s_consumers so that request IDs do not repeat
unit tests need to be fixed and gethwrappers need to be regenerated