fusdc: status manager fixups #10544
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 |
| @@ -14,8 +14,8 @@ import { PendingTxStatus } from '../constants.js'; | |||
| /** | |||
| * Create the key for the pendingTxs MapStore. | |||
| * | |||
| * 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 pendingTxs.get(key); | ||
| }, | ||
| }, | ||
| ); | ||
| 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
| @@ -111,7 +111,6 @@ export const contract = async (zcf, privateArgs, zone, tools) => { | |||
| 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;?
| '"[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.
| 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
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