fusdc: status manager fixups#10544
Conversation
0xpatrickdev
force-pushed
the
pc/status-manager-fixups
branch
from
b4499ef to
8a09c77
Compare
Deploying agoric-sdk with
|
| Latest commit: |
2ba8051
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://67f80d13.agoric-sdk.pages.dev |
| Branch Preview URL: | https://pc-status-manager-fixups.agoric-sdk.pages.dev |
| * | ||
| * The key is a composite of `txHash` and `chainId` and not meant to be | ||
| * parsable. | ||
| * The key is a composite of `NobleAddress` and transaction `amount` and not |
There was a problem hiding this comment.
| * The key is a composite of `NobleAddress` and transaction `amount` and not | |
| * The key is a composite of Noble address `addr` and transaction `amount` and not |
| }, | ||
| }, | ||
| ); | ||
| return statusManager; |
There was a problem hiding this comment.
nit: i found the direct return a little clearer. This way I was looking for how statusManager is used
| const creatorFacet = zone.exo('Fast USDC Creator', undefined, { | ||
| /** @type {(operatorId: string) => Promise<Invitation<OperatorKit>>} */ | ||
| async makeOperatorInvitation(operatorId) { | ||
| // eslint-disable-next-line no-use-before-define |
There was a problem hiding this comment.
is this based on master? those lines shouldn't be there anymore
0xpatrickdev
force-pushed
the
pc/status-manager-fixups
branch
from
8a09c77 to
657bc34
Compare
| const { txHash, chainId } = evidence; | ||
| return `seenTx:${JSON.stringify([txHash, chainId])}`; | ||
| const { txHash } = evidence; | ||
| return `seenTx:${JSON.stringify([txHash])}`; |
There was a problem hiding this comment.
any reason not to just return txHash;?
| t.deepEqual(inspectLogs(1), [ | ||
| 'Advancer error:', | ||
| '"[Error: Transaction already seen: \\"seenTx:[\\\\\\"0xc81bc6105b60a234c7c50ac17816ebcd5561d366df8bf3be59ff387552761702\\\\\\",1]\\"]"', | ||
| '"[Error: Transaction already seen: \\"seenTx:[\\\\\\"0xc81bc6105b60a234c7c50ac17816ebcd5561d366df8bf3be59ff387552761702\\\\\\"]\\"]"', |
There was a problem hiding this comment.
In my book, if you can see a difference from a test using an external API, it's not a refactor.
But we don't seem to have a shared definition across the team. Oh well.
There was a problem hiding this comment.
I chose refactor since this part of the implementation is invisible to contract consumers and other exos in the contract. I can amend both of the refactors to feat, though.
There was a problem hiding this comment.
ah. so it's an internal API. fair enough.
samsiegart
left a comment
samsiegart
left a comment
There was a problem hiding this comment.
Approve with one optional suggestion
| const { txHash, chainId } = evidence; | ||
| return `seenTx:${JSON.stringify([txHash, chainId])}`; | ||
| const { txHash } = evidence; | ||
| return `seenTx:${JSON.stringify([txHash])}`; |
There was a problem hiding this comment.
Do we need the seenTx: part? At the least, we probably don't need to stringify [txHash], just append txHash right?
- no need to support legacy ethereum txs that do not include chain id in the tx evelope - aligns implementation with vstorage plans
- if we settle and there's no additional entries for a key, delete it instead of setting to an empty array - ensure always return 'No unsettled entry' error instead of sometimes 'key not found'
0xpatrickdev
force-pushed
the
pc/status-manager-fixups
branch
from
657bc34 to
2ba8051
Compare
4 participants
refs: #10389
Description
Minor bug fixes and improvements for status manager.
Security Considerations
N/A
Scaling Considerations
Ensures mapStore entries with empty values are removed
Documentation Considerations
Makes Status Manager state diagrams more accessible (top level README.md)
Testing Considerations
Includes test for bug fixed
Upgrade Considerations
None, unreleased code