Review

[anilhelvaci]

Writing stuff that's on the top of my head here so I won't forget them

[anilhelvaci]

Minter shouldn't be injected through terms

The ist-forwarder contract expects a mint object through its terms. Since terms are public, anyone who has a reference to the instance of this contract can access the mint object. To avoid this security risk, my suggestion is to inject the mint object using privateArgs.

From this;

const { publicFacet } = await E(zoe).startInstance(
    installation,
    {},
    { board: fakeBoard, namesByAddress: fakeNamesByAddress, network, remoteConnectionId: "connection-0", psm, minter },
  );

To this;

const { publicFacet } = await E(zoe).startInstance(
    installation,
    {},
    { board: fakeBoard, namesByAddress: fakeNamesByAddress, network, remoteConnectionId: "connection-0", psm },
    { minter },
  );

[anilhelvaci]

Too many awaits in the contract code

When going over the code, I've noticed that there are too many await statements being used where the same thing could be achieved using await Promise.all([]). I'll just drop a few examples here.

// contract.js line 53

const issuer = await E(minter).getIssuer();
const brand = await E(issuer).getBrand();
const coins = await E(minter).mintPayment(AmountMath.make(brand, Nat(Number(value))));

Why not this?

const issuerP = E(minter).getIssuer();
const brandP = E(issuerP).getBrand();
const [issuer, brand] = await Promise.all([
  issuerP,
  brandP,
]);

const coins = await E(minter).mintPayment(AmountMath.make(brand, Nat(Number(value))));

Above is just an example, but I see big potential to reduce the number of awaits substantially.

Additional Note on Fetching Brand and Issuer

Since there will be one mint object per contract instance, why not fetch brand and issuer on the contract startup like:

const start = async (zcf, privateArgs) => {
  const { minter } = zcf.getTerms();

  const issuerP = E(minter).getIssuer();
  const brandP = E(issuerP).getBrand();
  const [ issuer, brand ] = await Promise.all([issuerP, brandP]);
}

[anilhelvaci]

Is there a test where we actually send/receive assets through IBC?

[anilhelvaci]

Is it possible enforce offer safety in either sendTransfer or onReceive and if possible both?

Below is the signature of sendTransfer;

/**
     * Swap IST into IBC ERTP asset. Then burn this ERTP asset and send to remote chain.
     *
     * @param {Payment} tokenIn IST amount in
     * @param {String} remoteDenom the remote denom to send back over channel
     * @param {String} receiver remote address receiver
     */
sendTransfer: async (tokenIn, remoteDenom, receiver)

To my knowledge, sending payments as method arguments is an anti-pattern. One way to avoid this is to escrow the payment in Zoe. Hence, I believe it would be a good improvement to turn sendTransfer to a Zoe offerHandler like:

const makeSendTransferInvitation = () => {
  const sendTransfer = (transferSeat, offerArgs) => {
    const { remoteDenom, receiver } = offerArgs;
    const { give: { IST: istAmountIn } } = transferSeat.getProposal();
    // Use offerTo to send offers to PSM
  };

  return zcf.makeInvitation(sendTransfer, 'Send Transfer');
} ;

In sendTransfer, the payout from PSM is burnt. Thus, actually getting the payout is required. So probably we have to do something like this before we send the offer via offerTo:

const { zcfSeat: mySeat, userSeat } = zcf.makeEmptySeatKit();
const { deposited } = await offerTo(.., .., .., .., .., mySeat); // Send mySeat as sixth argument so it receives the payout
await deposited;
mySeat.exit();
const payout = await E(userSeat).getPayout('Out')

As for the onReceive, I don't think using offerTo would make things easier or safer.

[anilhelvaci]

Why accept a mint object and not use zcfMint?

I assume this is probably related to the fact that the anchor asset in PSM can be minted from other sources that also listen for IBC transactions and the forwarder contract will not be the only one minting that anchor asset.

[anilhelvaci]

Questions to Agoric Engineering Team

Do you need to see this contract works as desired on an actual node(probably local) or unit tests are enough?

Yes, probably we want to see them install it with a core-eval on devnet & show it working on that network. My team can help with writing the core-eval and explaining the process in how to install it.

Do you want this bounty to work on master? Or do we have commit hash we can use?

The mainnet1B release (to be published on https://github.com/Agoric/agoric-sdk/releases in the next 48 hours) is good to target.

Jeet forwarded above questions to the engineering team and these are replies.

cc @jeetraut

[schnetzlerjoe]

Minter shouldn't be injected through terms

The ist-forwarder contract expects a mint object through its terms. Since terms are public, anyone who has a reference to the instance of this contract can access the mint object. To avoid this security risk, my suggestion is to inject the mint object using privateArgs.

From this;

const { publicFacet } = await E(zoe).startInstance(
    installation,
    {},
    { board: fakeBoard, namesByAddress: fakeNamesByAddress, network, remoteConnectionId: "connection-0", psm, minter },
  );

To this;

const { publicFacet } = await E(zoe).startInstance(
    installation,
    {},
    { board: fakeBoard, namesByAddress: fakeNamesByAddress, network, remoteConnectionId: "connection-0", psm },
    { minter },
  );

It was a public object but good point. Rather safe then sorry I changed it to private arg.

[schnetzlerjoe]

Is there a test where we actually send/receive assets through IBC?

Thats what the test-contract.js does. It sends mock IBC txs for testing.

[schnetzlerjoe]

Why accept a mint object and not use zcfMint?

I assume this is probably related to the fact that the anchor asset in PSM can be minted from other sources that also listen for IBC transactions and the forwarder contract will not be the only one minting that anchor asset.

This is exactly right/why

[schnetzlerjoe]

Too many awaits in the contract code

When going over the code, I've noticed that there are too many await statements being used where the same thing could be achieved using await Promise.all([]). I'll just drop a few examples here.

// contract.js line 53

const issuer = await E(minter).getIssuer();
const brand = await E(issuer).getBrand();
const coins = await E(minter).mintPayment(AmountMath.make(brand, Nat(Number(value))));

Why not this?

const issuerP = E(minter).getIssuer();
const brandP = E(issuerP).getBrand();
const [issuer, brand] = await Promise.all([
  issuerP,
  brandP,
]);

const coins = await E(minter).mintPayment(AmountMath.make(brand, Nat(Number(value))));

Above is just an example, but I see big potential to reduce the number of awaits substantially.

Additional Note on Fetching Brand and Issuer

Since there will be one mint object per contract instance, why not fetch brand and issuer on the contract startup like:

const start = async (zcf, privateArgs) => {
  const { minter } = zcf.getTerms();

  const issuerP = E(minter).getIssuer();
  const brandP = E(issuerP).getBrand();
  const [ issuer, brand ] = await Promise.all([issuerP, brandP]);
}

Debugging purposes. That is why. I have found it much easier (although still so hard) to find where bugs are occurring within Swingset Kernal when things are as separated as possible. I can reduce the awaits if its really necessary but there is no real benefit or negative. The kernel operates at the same speed and runtime regardless.

[schnetzlerjoe]

I believe I did this because I had to cause I was running into problems because it results in "double escrowing". So escrowing an escrow. You are escrowing through zoe offers for the psm so by doing that in our own logic results in that double escrow. How do you suggest we do that while still obtaining the payment and properly erroring/exiting on issues? Do you mind submitting as a PR your changes?

[anilhelvaci]

Is there a test where we actually send/receive assets through IBC?

Thats what the test-contract.js does. It sends mock IBC txs for testing.

Sorry for the lack of clarity in the question. What I meant was; is there an example where we can send/receive assets with real IBC txs instead of mock ones? I believe this question also relates to Agoric engineering team's response to my earlier question:

Do you need to see this contract works as desired on an actual node(probably local) or unit tests are enough?

Yes, probably we want to see them install it with a core-eval on devnet & show it working on that network. My team can help with writing the core-eval and explaining the process in how to install it.

Personally I'm good with seeing it work on a local node running the mainnet-1B as well. I don't know which one @jeetraut prefers. If you decide to go for the devnet I can help with the core-eval as well.

[anilhelvaci]

Do you want this bounty to work on master? Or do we have commit hash we can use?

The mainnet1B release (to be published on https://github.com/Agoric/agoric-sdk/releases in the next 48 hours) is good to target.

I'm putting this as a separate comment so it wouldn't slip from our attention. Looks like we need to make this work on mainnet-1B.

cc @jeetraut

[anilhelvaci]

I believe I did this because I had to cause I was running into problems because it results in "double escrowing". So escrowing an escrow. You are escrowing through zoe offers for the psm so by doing that in our own logic results in that double escrow. How do you suggest we do that while still obtaining the payment and properly erroring/exiting on issues? Do you mind submitting as a PR your changes?

Disclaimer: I'm answering above comment thinking that we're in the context of this comment I made earlier.

I think the offer safety is required because:

• Whoever is calling ist-forwarder contract's sendTransfer method has to trust it with their payments or if we're calling this method from an off-chain environment the user has to trust the client code to actually send their payment as the method argument. This feels like a stretch to me.

What we could do instead is sendTransfer uses offer safety and we pass the zoe escrow to PSM so there wouldn't be a double escrowing. I'll try to submit a PR for this later today.

[schnetzlerjoe]

Is there a test where we actually send/receive assets through IBC?

Thats what the test-contract.js does. It sends mock IBC txs for testing.

Sorry for the lack of clarity in the question. What I meant was; is there an example where we can send/receive assets with real IBC txs instead of mock ones? I believe this question also relates to Agoric engineering team's response to my earlier question:

Do you need to see this contract works as desired on an actual node(probably local) or unit tests are enough?

Yes, probably we want to see them install it with a core-eval on devnet & show it working on that network. My team can help with writing the core-eval and explaining the process in how to install it.

Personally I'm good with seeing it work on a local node running the mainnet-1B as well. I don't know which one @jeetraut prefers. If you decide to go for the devnet I can help with the core-eval as well.

I don't think I still understand. The packets being received within the unit tests are "real IBC packets" as in they are the same exact IBC tests you use in IBC testing just built in JS instead of Golang.

If you mean as in running on Mainnet/Testnet with a remote live chain then of course you would just replicate exactly what the test is doing but instead do so on Agoric mainnet/test and instead of using a fake IBC chain mimicked in JS do with a real remote chain by sending and IBC transfer. This requires privileged access to many things psm related.

Is that what you mean?

[schnetzlerjoe]

I believe I did this because I had to cause I was running into problems because it results in "double escrowing". So escrowing an escrow. You are escrowing through zoe offers for the psm so by doing that in our own logic results in that double escrow. How do you suggest we do that while still obtaining the payment and properly erroring/exiting on issues? Do you mind submitting as a PR your changes?

Disclaimer: I'm answering above comment thinking that we're in the context of this comment I made earlier.

I think the offer safety is required because:

• Whoever is calling ist-forwarder contract's sendTransfer method has to trust it with their payments or if we're calling this method from an off-chain environment the user has to trust the client code to actually send their payment as the method argument. This feels like a stretch to me.

What we could do instead is sendTransfer uses offer safety and we pass the zoe escrow to PSM so there wouldn't be a double escrowing. I'll try to submit a PR for this later today.

That sounds like that would work. When you submit that I can check that out!

[anilhelvaci]

Too many awaits in the contract code

When going over the code, I've noticed that there are too many await statements being used where the same thing could be achieved using await Promise.all([]). I'll just drop a few examples here.

// contract.js line 53

const issuer = await E(minter).getIssuer();
const brand = await E(issuer).getBrand();
const coins = await E(minter).mintPayment(AmountMath.make(brand, Nat(Number(value))));

Why not this?

const issuerP = E(minter).getIssuer();
const brandP = E(issuerP).getBrand();
const [issuer, brand] = await Promise.all([
  issuerP,
  brandP,
]);

const coins = await E(minter).mintPayment(AmountMath.make(brand, Nat(Number(value))));

Above is just an example, but I see big potential to reduce the number of awaits substantially.

Additional Note on Fetching Brand and Issuer

Since there will be one mint object per contract instance, why not fetch brand and issuer on the contract startup like:

const start = async (zcf, privateArgs) => {
  const { minter } = zcf.getTerms();

  const issuerP = E(minter).getIssuer();
  const brandP = E(issuerP).getBrand();
  const [ issuer, brand ] = await Promise.all([issuerP, brandP]);
}

Debugging purposes. That is why. I have found it much easier (although still so hard) to find where bugs are occurring within Swingset Kernal when things are as separated as possible. I can reduce the awaits if its really necessary but there is no real benefit or negative. The kernel operates at the same speed and runtime regardless.

I'm not a kernel expert so I asked this to @arirubinstein and @dtribble in Prague last week. Long story short, as long as you're talking to objects that are in the same vat as you, yeah you're right the performance is not affected. Since unit tests are mock environments the performance looks good. But I doubt this will be the case when running on a dev or prod environment.

[anilhelvaci]

Is there a test where we actually send/receive assets through IBC?

Thats what the test-contract.js does. It sends mock IBC txs for testing.

Sorry for the lack of clarity in the question. What I meant was; is there an example where we can send/receive assets with real IBC txs instead of mock ones? I believe this question also relates to Agoric engineering team's response to my earlier question:

Do you need to see this contract works as desired on an actual node(probably local) or unit tests are enough?

Yes, probably we want to see them install it with a core-eval on devnet & show it working on that network. My team can help with writing the core-eval and explaining the process in how to install it.

Personally I'm good with seeing it work on a local node running the mainnet-1B as well. I don't know which one @jeetraut prefers. If you decide to go for the devnet I can help with the core-eval as well.

I don't think I still understand. The packets being received within the unit tests are "real IBC packets" as in they are the same exact IBC tests you use in IBC testing just built in JS instead of Golang.

If you mean as in running on Mainnet/Testnet with a remote live chain then of course you would just replicate exactly what the test is doing but instead do so on Agoric mainnet/test and instead of using a fake IBC chain mimicked in JS do with a real remote chain by sending and IBC transfer. This requires privileged access to many things psm related.

Is that what you mean?

By "real IBC txs" , just like Agoric team said, to see those transactions happen between two chains and the agoric one running mainnet-1B. So you're right that was that I meant.

As for the PSM privileges, that is what core-eval is for.