-
Notifications
You must be signed in to change notification settings - Fork 214
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
feat(orchestration): deposit ERTP payment to ICA #9342
Conversation
- refs: 9193
- uses a LocalChainAccount to accept an ERTP Payment and transfer it to an ICA account - uses ChainTimerService to create a timeout timestamp for ibc/MsgTransfer
Deploying agoric-sdk with Cloudflare Pages
|
trace('Transferring funds to ICA'); | ||
/** | ||
* // TODO can we infer `/ibc.applications.transfer.v1.MsgTransferResponse`? | ||
* @type {unknown[]} |
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 current (unsuccessful) attempt at this:
packages/cosmic-proto/src/helpers.ts
git diff packages/cosmic-proto/src/helpers.ts
diff --git a/packages/cosmic-proto/src/helpers.ts b/packages/cosmic-proto/src/helpers.ts
index ca192be60..00ebfea30 100644
--- a/packages/cosmic-proto/src/helpers.ts
+++ b/packages/cosmic-proto/src/helpers.ts
@@ -1,6 +1,15 @@
-import type { QueryAllBalancesRequest } from './codegen/cosmos/bank/v1beta1/query.js';
-import type { MsgSend } from './codegen/cosmos/bank/v1beta1/tx.js';
-import type { MsgDelegate } from './codegen/cosmos/staking/v1beta1/tx.js';
+import type {
+ QueryAllBalancesRequest,
+ QueryAllBalancesResponse,
+} from './codegen/cosmos/bank/v1beta1/query.js';
+import type {
+ MsgSend,
+ MsgSendResponse,
+} from './codegen/cosmos/bank/v1beta1/tx.js';
+import type {
+ MsgDelegate,
+ MsgDelegateResponse,
+} from './codegen/cosmos/staking/v1beta1/tx.js';
import { RequestQuery } from './codegen/tendermint/abci/types.js';
import type { Any } from './codegen/google/protobuf/any.js';
import {
@@ -13,17 +22,32 @@ import {
* `unknown` but it's actually this. The `value` string is a base64 encoding of
* the bytes array.
*/
-export type AnyJson = { typeUrl: string; value: string };
-
-// TODO codegen this by modifying Telescope
+export type AnyJson = {
+ typeUrl: string;
+ value: string;
+};
export type Proto3Shape = {
'/cosmos.bank.v1beta1.MsgSend': MsgSend;
+ '/cosmos.bank.v1beta1.MsgSendResponse': MsgSendResponse;
'/cosmos.bank.v1beta1.QueryAllBalancesRequest': QueryAllBalancesRequest;
+ '/cosmos.bank.v1beta1.QueryAllBalancesResponse': QueryAllBalancesResponse;
'/cosmos.staking.v1beta1.MsgDelegate': MsgDelegate;
+ '/cosmos.staking.v1beta1.MsgDelegateResponse': MsgDelegateResponse;
'/ibc.applications.transfer.v1.MsgTransfer': MsgTransfer;
'/ibc.applications.transfer.v1.MsgTransferResponse': MsgTransferResponse;
};
+export type ResponseType<T> =
+ T extends '/ibc.applications.transfer.v1.MsgTransfer'
+ ? '/ibc.applications.transfer.v1.MsgTransferResponse'
+ : T extends '/cosmos.bank.v1beta1.MsgSend'
+ ? '/cosmos.bank.v1beta1.MsgSendResponse'
+ : T extends '/cosmos.staking.v1beta1.MsgDelegate'
+ ? '/cosmos.staking.v1beta1.MsgDelegateResponse'
+ : T extends '/cosmos.bank.v1beta1.QueryAllBalancesRequest'
+ ? '/cosmos.bank.v1beta1.QueryAllBalancesResponse'
+ : never;
+
/**
* The encoding introduced in Protobuf 3 for Any that can be serialized to JSON.
*
packages/vats/src/localchain.js
git diff packages/vats/src/localchain.js
diff --git a/packages/vats/src/localchain.js b/packages/vats/src/localchain.js
index 5a07e8ad3..6ab60c379 100644
--- a/packages/vats/src/localchain.js
+++ b/packages/vats/src/localchain.js
@@ -5,6 +5,8 @@ import { AmountShape } from '@agoric/ertp';
const { Fail, bare } = assert;
+/** @import {TypedJson, ResponseType, Proto3Shape} from '@agoric/cosmic-proto'; */
+
/**
* @typedef {{
* system: import('./types.js').ScopedBridgeManager;
@@ -82,8 +84,9 @@ const prepareLocalChainAccount = zone =>
return E(purse).withdraw(amount);
},
/**
- * @param {import('@agoric/cosmic-proto').TypedJson<unknown>[]} messages
- * @returns {Promise<import('@agoric/cosmic-proto').TypedJson[]>}
+ * @template {keyof Proto3Shape} T
+ * @param {TypedJson<T>[]} messages
+ * @returns {Promise<TypedJson<ResponseType<T>>[]>}
*/
async executeTx(messages) {
const { address, powers } = this.state;
@@ -178,8 +181,8 @@ const prepareLocalChain = (zone, makeAccount) =>
* the query fails. Otherwise, return the response as a JSON-compatible
* object.
*
- * @param {import('@agoric/cosmic-proto').TypedJson} request
- * @returns {Promise<import('@agoric/cosmic-proto').TypedJson>}
+ * @param {TypedJson} request
+ * @returns {Promise<TypedJson>}
*/
async query(request) {
const requests = harden([request]);
@@ -197,11 +200,11 @@ const prepareLocalChain = (zone, makeAccount) =>
* system error, will return all results to indicate their success or
* failure.
*
- * @param {import('@agoric/cosmic-proto').TypedJson[]} requests
+ * @param {TypedJson[]} requests
* @returns {Promise<
* {
* error?: string;
- * reply: import('@agoric/cosmic-proto').TypedJson;
+ * reply: TypedJson;
* }[]
* >}
*/
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.
]); | ||
trace('MsgTransfer result', result); | ||
|
||
return /** @type {{ sequence: number }} */ (result); |
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.
Was expecting to see a bigint
or string
here. Surprised to see a number
, but this is what I observed with local chains.
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.
is that observed data captured in a unit test?
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.
It's currently captured in packages/boot/tools/supports.ts
: https://github.com/Agoric/agoric-sdk/pull/9342/files#diff-5f4f231637e90b9cdfecc29b28c80beb222607ad154bbc266cbf52c6a7c246e9R438-R446
I considered adding a new type helper to @agoric/cosmic-proto
that's like JsonSafe
, but wanted to be sure of this behavior before proceeding. @michaelfig - do you have any insights here? Are bigint
s cast to number
s for the LocalChainAccount
implementation?
/** | ||
* see stakingAccountKit.js for an example until #9212 | ||
* @param {Payment} payment | ||
*/ |
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.
Given Scaling Considerations, we may want to reconsider including deposit()
on ChainAccount
.
@@ -5,6 +5,8 @@ import { AmountShape } from '@agoric/ertp'; | |||
|
|||
const { Fail, bare } = assert; | |||
|
|||
/** @import {TypedJson, ResponseType, Proto3Shape} from '@agoric/cosmic-proto'; */ |
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.
/** @import {TypedJson, ResponseType, Proto3Shape} from '@agoric/cosmic-proto'; */ | |
/** @import {TypedJson, Proto3Shape} from '@agoric/cosmic-proto'; */ |
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.
i wouldn't worry about extra types for now. especially because ambients can make it look like imports that aren't used that actually are during build time
"sourceChannelId": "channel-1", | ||
"sourcePortId": "transfer" | ||
}, | ||
"icqEnabled": false |
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.
set true
for tests, where query responses are mocked, but won't work on a local simulated chain
@@ -295,6 +295,7 @@ export const makeSwingsetTestKit = async ( | |||
|
|||
let inbound; | |||
let ibcSequenceNonce = 0; | |||
let icaExecuteTxSequence = 0; |
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.
let icaExecuteTxSequence = 0; | |
let lcaExecuteTxSequence = 0; |
@@ -215,3 +339,5 @@ test.serial('stakeAtom - smart wallet', async t => { | |||
'delegate fails with invalid validator', | |||
); | |||
}); | |||
|
|||
test.todo('deposit to LCA fails, payment should be returned'); |
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.
Currently only simulating failed LCA -> ICA transfer. This captures the last nested block of the Deposit()
try / catch
, but we have nothing testing the first depositToSeat
, which depends on await Object.values(payments)[0]
. Suggestions on how to simulate this welcome.
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.
time for a meeting; better share my notes so far
@@ -77,6 +77,11 @@ export const AmountShape = harden({ | |||
value: AmountValueShape, | |||
}); | |||
|
|||
export const NatAmountShape = harden({ | |||
brand: BrandShape, | |||
value: NatValueShape, |
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.
there's a NatValueShape
? even though it's the same as M.nat()
? odd.
@@ -1,3 +1,4 @@ | |||
/* eslint-disable camelcase */ |
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.
it's not obvious why camelcase
should be disabled. is it easy to add a few words? no big deal for test code, I guess
@@ -150,6 +151,7 @@ test.serial('stakeAtom - smart wallet', async t => { | |||
'agoric1testStakAtom', | |||
); | |||
|
|||
// 1. Make Account |
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.
consider turning such comments into t.log()
calls.
odd... makeAcountInvitationMaker
? with 1 c? oops!
}, | ||
proposal: { | ||
give: { | ||
// @ts-expect-error BoardRemote is not assignable to Brand<any> |
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.
I/we really should fix that. the getBoardId()
kludge is a bad idea. the mapping from value to slot (board id) belongs in a table external to the object; we learned this in makeClientMarshaller but we haven't back-ported it all the way to here.
IOU an issue, I guess.
}); | ||
t.like(wd.getLatestUpdateRecord(), { | ||
status: { id: 'request-deposit-success', numWantsSatisfied: 1 }, | ||
}); |
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.
Hm. I'd like to see a check that the money got there. Can we capture calls across the bridge and check for a suitable one here?
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.
The best we can do in this environment is mock, which doesn't give the highest confidence or seem worth the cost. I am going to pursue better unit testing at the exo level per your suggestion to get more confidence around these failure cases
}, | ||
}), | ||
{ | ||
message: 'Deposit failed, payment returned.', |
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.
again, it would be good to have evidence that the right vbank messages went over the bridge
ah... but we do have payouts
below
bondDenom = 'uatom', | ||
bondDenomLocal = 'ibc/C4CFF46FD6DE35CA4CF4CE031E643C8FDC9BA4B99AE598E9B0ED98FE3A2319F9', |
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.
it would be nice to get this from hashing... or to test that this matches what we get from hashing
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.
Agree. Something i think we'll tackle as part of #9211
@@ -50,25 +73,40 @@ export const start = async (zcf, privateArgs, baggage) => { | |||
zcf, | |||
); | |||
|
|||
/** @type {BrandToIssuer} */ | |||
const brandToIssuer = zone.mapStore('brandToIssuer'); |
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.
a durable store is over-kill for this. it costs a syscall for each access.
const icqConnection = icqEnabled | ||
? await E(orchestration).provideICQConnection(controllerConnectionId) | ||
: undefined; |
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.
Maybe it's ok by the new await safety style, but I've picked up an aversion to code that sometimes represents a turn boundary and sometimes doesn't.
const icqConnection = icqEnabled | |
? await E(orchestration).provideICQConnection(controllerConnectionId) | |
: undefined; | |
const icqConnection = await (icqEnabled | |
? E(orchestration).provideICQConnection(controllerConnectionId) | |
: undefined); |
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.
I'm also reminded to check for a clear commit point, as in the prepare / commit pattern.
If makeAccount()
succeeds but provideICQConnection()
fails, should we release the account?
@@ -77,6 +77,11 @@ export const AmountShape = harden({ | |||
value: AmountValueShape, | |||
}); | |||
|
|||
export const NatAmountShape = harden({ |
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.
export const NatAmountShape = harden({ | |
/** @see {makeNatAmountShape} to match a specific brand */ | |
export const NatAmountShape = harden({ |
numerator: AmountShape, | ||
denominator: AmountShape, |
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.
numerator: AmountShape, | |
denominator: AmountShape, | |
numerator: NatAmountShape, | |
denominator: NatAmountShape, |
|
||
// 2. Deposit to Account | ||
await wd.executeOffer({ | ||
id: 'request-deposit-success', |
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.
success is independent of the offer identity
id: 'request-deposit-success', | |
id: 'request-deposit', |
reading further I see you have other variants. Still I think success or failure are independent of identity. E.g.
request-deposit
request-deposit-bad-want
request-deposit-two-gives
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.
I tripped over that name/id on 1st reading too.
status: { id: 'request-deposit-success', numWantsSatisfied: 1 }, | ||
}); | ||
|
||
await t.throwsAsync( |
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.
good to have these validation tests, but they don't need to integrate with smart-wallet. Spinning up a contract in isolation is a bit of work, I don't know that they even need to integrate with stakeAtom. Could these just be tests of the StakingAccountKit
exo? Those would run very fast
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.
Acknowledged and agree: #9342 (comment)
@@ -60,7 +61,8 @@ | |||
}, | |||
"files": [ | |||
"test/**/*.test.js", | |||
"test/**/*.test.ts" | |||
"test/**/*.test.ts", | |||
"test/**/test-*.js" |
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.
reminds me to revive #8653
// messages from the LCA (e.g., withdraw funds) | ||
await E(localAccount).deposit(payment); | ||
|
||
const timeoutTimestamp = |
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.
this is a conditional await. I think it's safe because of the await above it, but please add a // @jessie-check
at the top to be sure. all modules that run in a vat should pass jessie-check
trace('Transferring funds to ICA'); | ||
/** | ||
* // TODO can we infer `/ibc.applications.transfer.v1.MsgTransferResponse`? | ||
* @type {unknown[]} |
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.
const chainTimerService = await chainTimerServiceP; | ||
const chainTimerBrand = await E(chainTimerService).getTimerBrand(); |
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.
why not pipeline?
const chainTimerService = await chainTimerServiceP; | |
const chainTimerBrand = await E(chainTimerService).getTimerBrand(); | |
const chainTimerBrand = await E(chainTimerServiceP).getTimerBrand(); |
@@ -266,7 +267,7 @@ export interface ChainAccount { | |||
opts?: Partial<Omit<TxBody, 'messages'>>, | |||
) => Promise<string>; | |||
/** deposit payment from zoe to the account*/ | |||
deposit: (payment: Payment) => Promise<void>; | |||
deposit: (payment: Payment) => Promise<{ sequence: number }>; |
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.
@dtribble wants to review changes to this API
@@ -5,6 +5,8 @@ import { AmountShape } from '@agoric/ertp'; | |||
|
|||
const { Fail, bare } = assert; | |||
|
|||
/** @import {TypedJson, ResponseType, Proto3Shape} from '@agoric/cosmic-proto'; */ |
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.
i wouldn't worry about extra types for now. especially because ambients can make it look like imports that aren't used that actually are during build time
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.
I'm running out of steam for today.
But at least the bankManager change is critical.
this.state.brandToIssuer.has(giveAmount.brand) || | ||
Fail`${giveAmount.brand} not registered`; |
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.
Contracts can rely on Zoe to ensure that brands in proposals are registered.
await depositToSeat(zcf, seat, give, payments); | ||
throw Fail`Deposit failed, payment returned.`; | ||
} catch (error) { | ||
if (error.message.includes('not a live paymen')) { |
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.
Our style for errors and control flow is to not peek at errors.
Why do we care what sort of error it is?
await depositToSeat(zcf, seat, give, payments); | ||
throw Fail`Deposit failed, payment returned.`; | ||
} catch (error) { | ||
if (error.message.includes('not a live paymen')) { |
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.
Also, are we testing all these failure modes?
I wonder if it would help to hoist much of this logic to a function that we can unit test.
/** | ||
* - `give` allows any `Nat` `issuerKeyword` record. Must be exactly one entry. | ||
* - `exit` must be `{ waived: null }` | ||
* - `want` must be empty | ||
*/ | ||
export const DepositProposalShape = M.splitRecord( |
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.
waived: null
merits some explanation: once somebody makes such an offer, undoing it may be infeasible, so the client can't honor exit requests; clients must waive their right to exit.
docs should bear a WARNING notice about lack of offer safety
maybe the name should be scarier too... GiveAndHope
:)
maybe these docs belong on the deposit()
method and/or the Deposit
invitationMaker.
const { address, powers } = this.state; | ||
const bankManager = getPower(powers, 'bankManager'); | ||
|
||
const acctsBank = E(bankManager).getBankForAddress(address); |
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.
This getBankForAddress()
call should happen before this account object is created. No account should have access to everybody else's purses. critical
(Didn't I give this feedback a while ago? hm.)
Refs: #9342 ## Description Make it so executeTx gives the proper response message for each request message. ### Security Considerations <!-- Does this change introduce new assumptions or dependencies that, if violated, could introduce security vulnerabilities? How does this PR change the boundaries between mutually-suspicious components? What new authorities are introduced by this change, perhaps by new API calls? --> ### Scaling Considerations <!-- Does this change require or encourage significant increase in consumption of CPU cycles, RAM, on-chain storage, message exchanges, or other scarce resources? If so, can that be prevented or mitigated? --> ### Documentation Considerations <!-- Give our docs folks some hints about what needs to be described to downstream users. Backwards compatibility: what happens to existing data or deployments when this code is shipped? Do we need to instruct users to do something to upgrade their saved data? If there is no upgrade path possible, how bad will that be for users? --> ### Testing Considerations <!-- Every PR should of course come with tests of its own functionality. What additional tests are still needed beyond those unit tests? How does this affect CI, other test automation, or the testnet? --> ### Upgrade Considerations <!-- What aspects of this PR are relevant to upgrading live production systems, and how should they be addressed? -->
refs: #9342, #9193 stacked on - #9353 ## Description Provide localchain accounts only with the `bank` for the relevant address, rather than giving them access to all accounts in the form of the `bankManager`. earlier discussion: https://github.com/Agoric/agoric-sdk/pull/9342/files#r1594791187 ### Security Considerations bankManager was excess authority ### Scaling / Documentation / Testing Considerations Nothing significant: no scaling changes; existing docs/tests suffice. ### Upgrade Considerations code is not released / deployed
closes: #9193
closes: #9042
Description
.deposit()
method tostakingAccountHolder
to enable ERTP payment deposits to an Interchain Account. Flow:LocalChainAccount
is created andPayment
is deposited thereLocalChainAccount
sends ibc/MsgTransfer
toChainAccount
ChainTimerService
is used to create a timeout timestamp. We take the current time and add five minutes to it.withdraw()
method toLocalChainAccount
bondDenom
until bidirectional lookup of denoms / brands for IBC #9211, orchestration - chain - provide well-known chain #9063 (need to determinedenom
fromAmount
/Brand
Security Considerations
Offer Safety:
withdrawFromSeat
is needed to retrieve a live payment from a seat. Offer safety is upheld because the user is not allowed to request a Want, but we need to proceed carefully. There is a chance the deposit fails along the way, and we need to ensure the payment can be recovered and deposited back to a users seat.chainTimerService
is needed to create a timeout parameter forMsgTransfer
Scaling Considerations
In order for a
ChainAccount
to have adeposit()
functionality, aLocalChainAccount
needs to be created. This could lead to a lot of LCA's.withdraw
,.executeTx
,.query
)TimerService is needed to create a
MsgTransfer
timeout parameter. See Orchestration: optimizetimeoutHeight
parameter for transactions, queries, and transfers #9324 for more detailsDocumentation Considerations
Testing Considerations
I wanted to add more explicit tests for the functionality added to
LocalChainAccount
, but struggled to find a place to add them (and get access to a mint in bootstrap tests). Bothdeposit()
andwithdraw()
are tested indirectly by tests inboot/../test-orchestration.ts
I would like feedback on the imperative
.deposit()
flow - what happens if this throws? Can we assume the callers supplied payment is still live if it wasn't deposited to the LCA?Upgrade Considerations