search

near-daos / sputnik-dao-contract

Smart contracts for https://app.astrodao.com
https://astrodao.com/
MIT License
107 stars 76 forks source link

fix: fix exceeded the prepaid gas when upgrade remote contract #197

Open fospring opened 1 year ago

fospring commented 1 year ago

When GAS_FOR_UPGRADE_REMOTE_DEPLOY is 15T, upgrade remote contract will throw error of Exceeded the prepaid gas. Updated GAS_FOR_UPGRADE_REMOTE_DEPLOY to 25T would throw same error, So I updated it to 30T.

This is the failed tx when GAS_FOR_UPGRADE_REMOTE_DEPLOY is 25T: https://explorer.testnet.near.org/transactions/AR5j8Dz3Ts4ZDKXr1Zd5Eq7VGeS23AmWDtpjhih2wq2m

agileurbanite commented 1 year ago

But I believe you'll need @ilblackdragon approval given that he's the main contributor to sputnik atm.

fospring commented 1 year ago

@ilblackdragon can you pls review this MR?

ilblackdragon commented 1 year ago

I'm having trouble opening the execution plan for this transaction (https://testnet.nearblocks.io/txns/AR5j8Dz3Ts4ZDKXr1Zd5Eq7VGeS23AmWDtpjhih2wq2m#execution)

Mostly question is why this is needed, because this change may be hiding another issue. LEFTOVER parameters are to cover costs of calling promise_function_call itself and returning Promise and it should be not be taking this much gas. It shouldn't even be 15Tg in the first place.

There is also new API in 4.0 SDK via https://github.com/near/NEPs/pull/264 that allows to not have this at all, at it calculates leftover gas after function already completed: https://github.com/near/near-sdk-rs/issues/740

If we are updating this, let's switch to this API given sputnikdao2 is already on 4.0 sdk.

fospring commented 1 year ago

I'm having trouble opening the execution plan for this transaction (https://testnet.nearblocks.io/txns/AR5j8Dz3Ts4ZDKXr1Zd5Eq7VGeS23AmWDtpjhih2wq2m#execution)

Mostly question is why this is needed, because this change may be hiding another issue. LEFTOVER parameters are to cover costs of calling promise_function_call itself and returning Promise and it should be not be taking this much gas. It shouldn't even be 15Tg in the first place.

There is also new API in 4.0 SDK via near/NEPs#264 that allows to not have this at all, at it calculates leftover gas after function already completed: near/near-sdk-rs#740

If we are updating this, let's switch to this API given sputnikdao2 is already on 4.0 sdk.

fospring commented 1 year ago

I'm having trouble opening the execution plan for this transaction (https://testnet.nearblocks.io/txns/AR5j8Dz3Ts4ZDKXr1Zd5Eq7VGeS23AmWDtpjhih2wq2m#execution)

Mostly question is why this is needed, because this change may be hiding another issue. LEFTOVER parameters are to cover costs of calling promise_function_call itself and returning Promise and it should be not be taking this much gas. It shouldn't even be 15Tg in the first place.

There is also new API in 4.0 SDK via near/NEPs#264 that allows to not have this at all, at it calculates leftover gas after function already completed: near/near-sdk-rs#740

If we are updating this, let's switch to this API given sputnikdao2 is already on 4.0 sdk.

May be there are some trouble for FE in query execution plan: https://testnet.nearblocks.io/txns/AR5j8Dz3Ts4ZDKXr1Zd5Eq7VGeS23AmWDtpjhih2wq2m# image

agileurbanite commented 1 year ago

We need to migrate sputnikdao2 to the latest sdk as it's not even on 4.0 it's currently on 4.0.0-pre.4 which is not even defined as a release version on github: https://github.com/near/near-sdk-rs/releases?expanded=true&page=2&q=4.0.0-pre.4

But strangely there are rust docs on it: https://docs.rs/near-sdk/4.0.0-pre.4/near_sdk/index.html

Either way, I'm not sure https://github.com/near/NEPs/pull/264 was even introduced in 4.0.0-pre.5 as it looks like it was pretty of the changes for 4.0.0 stable release here: https://github.com/near/near-sdk-rs/releases/tag/4.0.0

What does this mean?

Well to migrate sputnikdao2 to >= 4.0.0 a few things need to happen:

  1. Migrate to the new rust SDK version and update deprecated methods
  2. Remove near-sdk-sim and migrate to workspaces-rs

The above is covered in: https://github.com/near-daos/sputnik-dao-contract/issues/198

Overall, however, my perception of the task is that it seems like a larger effort.

@ilblackdragon do you have concerns with increasing FACTORY_UPDATE_GAS_LEFTOVER as a stop gap for now and then during the follow up with updating the SDK we can re-evaluate the need for this as part of that effort.

Also @fospring to @ilblackdragon point, I do believe you need to re-evaluate your implementation as it should not take 15Tg in the first place. Can you share pseudo code of your implementation or your execution plan in detail?

fospring commented 1 year ago

We need to migrate sputnikdao2 to the latest sdk as it's not even on 4.0 it's currently on 4.0.0-pre.4 which is not even defined as a release version on github: https://github.com/near/near-sdk-rs/releases?expanded=true&page=2&q=4.0.0-pre.4

But strangely there are rust docs on it: https://docs.rs/near-sdk/4.0.0-pre.4/near_sdk/index.html

Either way, I'm not sure near/NEPs#264 was even introduced in 4.0.0-pre.5 as it looks like it was pretty of the changes for 4.0.0 stable release here: https://github.com/near/near-sdk-rs/releases/tag/4.0.0

What does this mean?

Well to migrate sputnikdao2 to >= 4.0.0 a few things need to happen:

  1. Migrate to the new rust SDK version and update deprecated methods
  2. Remove near-sdk-sim and migrate to workspaces-rs

The above is covered in: #198

Overall, however, my perception of the task is that it seems like a larger effort.

@ilblackdragon do you have concerns with increasing FACTORY_UPDATE_GAS_LEFTOVER as a stop gap for now and then during the follow up with updating the SDK we can re-evaluate the need for this as part of that effort.

Also @fospring to @ilblackdragon point, I do believe you need to re-evaluate your implementation as it should not take 15Tg in the first place. Can you share pseudo code of your implementation or your execution plan in detail?

@agileurbanite @ilblackdragon My executions flows:

  1. create mock accounts:
  2. build dao : 
    git clone git@github.com:near-daos/sputnik-dao-contract.git
cargo build --release -p sputnikdao2 --target wasm32-unknown-unknown
  3. deploy dao contract(or1007owner-2.yongchun.testnet, owner of or1007-2.yongchun.testnet) by this transaction
  4. deploy smart contract to or1007-2.yongchun.testnet with owner or1007owner-2.yongchun.testnet by this transaction
  5. store-blob of smart contract, ready to upgrade smart contract of or1007-2.yongchun.testnet by this transaction, the store-blob's base58_crypto hash is Hy8JJwzpRvhnSnEoQnF1WvDU5kZFksAbiScXsDeuzLZg
  6. add proposal of remote upgrade or1007-2.yongchun.testnet smart contract, and act proposal:

I think I have tested that 15T gas LEFTOVER is not enough for action of UpgradeRemote.

fospring commented 1 year ago

We need to migrate sputnikdao2 to the latest sdk as it's not even on 4.0 it's currently on 4.0.0-pre.4 which is not even defined as a release version on github: https://github.com/near/near-sdk-rs/releases?expanded=true&page=2&q=4.0.0-pre.4

But strangely there are rust docs on it: https://docs.rs/near-sdk/4.0.0-pre.4/near_sdk/index.html

Either way, I'm not sure near/NEPs#264 was even introduced in 4.0.0-pre.5 as it looks like it was pretty of the changes for 4.0.0 stable release here: https://github.com/near/near-sdk-rs/releases/tag/4.0.0

What does this mean?

Well to migrate sputnikdao2 to >= 4.0.0 a few things need to happen:

  1. Migrate to the new rust SDK version and update deprecated methods
  2. Remove near-sdk-sim and migrate to workspaces-rs

The above is covered in: #198

Overall, however, my perception of the task is that it seems like a larger effort.

@ilblackdragon do you have concerns with increasing FACTORY_UPDATE_GAS_LEFTOVER as a stop gap for now and then during the follow up with updating the SDK we can re-evaluate the need for this as part of that effort.

Also @fospring to @ilblackdragon point, I do believe you need to re-evaluate your implementation as it should not take 15Tg in the first place. Can you share pseudo code of your implementation or your execution plan in detail?

@agileurbanite @ilblackdragon, Thank you for your reminder. I have added a new transaction test that when FACTORY_UPDATE_GAS_LEFTOVER is 15Tg, it's enough, so I reverted updation of FACTORY_UPDATE_GAS_LEFTOVER by this commit e2ab7ad036c0a842fd69ba05f7bceb12cc740f84

pls check again

fospring commented 1 year ago

We need to migrate sputnikdao2 to the latest sdk as it's not even on 4.0 it's currently on 4.0.0-pre.4 which is not even defined as a release version on github: https://github.com/near/near-sdk-rs/releases?expanded=true&page=2&q=4.0.0-pre.4

But strangely there are rust docs on it: https://docs.rs/near-sdk/4.0.0-pre.4/near_sdk/index.html

Either way, I'm not sure near/NEPs#264 was even introduced in 4.0.0-pre.5 as it looks like it was pretty of the changes for 4.0.0 stable release here: https://github.com/near/near-sdk-rs/releases/tag/4.0.0

What does this mean?

Well to migrate sputnikdao2 to >= 4.0.0 a few things need to happen:

  1. Migrate to the new rust SDK version and update deprecated methods
  2. Remove near-sdk-sim and migrate to workspaces-rs

The above is covered in: #198

Overall, however, my perception of the task is that it seems like a larger effort.

@ilblackdragon do you have concerns with increasing FACTORY_UPDATE_GAS_LEFTOVER as a stop gap for now and then during the follow up with updating the SDK we can re-evaluate the need for this as part of that effort.

Also @fospring to @ilblackdragon point, I do believe you need to re-evaluate your implementation as it should not take 15Tg in the first place. Can you share pseudo code of your implementation or your execution plan in detail?

@agileurbanite @ilblackdragon can we hotfix on this issue? https://explorer.testnet.near.org/transactions/3xRbdcbdQnC1jSo2arzzQ6bJDNs7PPdrLARmvYFbRL68