Factory update: Agoric validation, nonce tracking, no payload/gas #20
Conversation
rabi-siddique
force-pushed
the
rs-parse-and-emit-evm-info
branch
from
5e323c5 to
164e0ca
Compare
rabi-siddique
force-pushed
the
rs-parse-and-emit-evm-info
branch
from
164e0ca to
6a2fb63
Compare
|
refs: #11647 ## Description This PR does the following: - Maintain a `nonce` value axelar gmp account kit. - Add a method to retrieve and increment the current nonce. - Remove the previous logic of passing `gasAmount` via `offerArgs` - Update related tests accordingly. These changes are used primarily in the [`agoric-labs/agoric-to-axelar-local`](https://github.com/agoric-labs/agoric-to-axelar-local) repo, specifically supporting [PR #20](agoric-labs/agoric-to-axelar-local#20). The goal is to ensure smooth local development and verify cross-chain integration behavior. ### Security Considerations No new security-relevant assumptions are introduced by this change. ### Scaling Considerations Not applicable. This is an example contract intended for local development and testing, not production deployment. ### Documentation Considerations No documentation updates are necessary. ### Testing Considerations Current suite of tests are enough to verify the changes made. ### Upgrade Considerations Not applicable, as this is for local testing only.
| ).to.be.revertedWith("nonce already used by sender"); | ||
| }); | ||
|
|
||
| it("should revert if message comes from a chain besides agoric", async () => { |
There was a problem hiding this comment.
To what extent does this prevent forgery? Do we know that when it comes from Agoric, it's coming from an orchestration-controlled account? Or can anybody with a normal key-based account send such a message?
There was a problem hiding this comment.
I suppose if anyone wants to pay for such an account, we don't mind.
| .to.emit(factory, "NewWalletCreated") | ||
| .withArgs(expectedWalletAddress, nonce, sourceAddress, "agoric"); |
| } | ||
|
|
||
| contract Factory is AxelarExecutable { | ||
| using StringToAddress for string; |
There was a problem hiding this comment.
the production contract is in a __tests__ directory?
| // TODO: Should we consider limiting or cleaning this mapping to avoid unbounded growth? | ||
| mapping(string => mapping(uint256 => bool)) public usedNonces; |
There was a problem hiding this comment.
Do we need to track used nonces separately for each source address?
How about just 1 lastNonceUsed for the whole Factory contract?
The ymax contract can issue nonces that are unique across all portfolios (zone.mapStore('evmNonce') with just 1 key such as it or theNonce).
If somebody wants to track a different sequence of nonces they can deploy a new instance of this Factory contract, I suppose.
There was a problem hiding this comment.
now I see why we need to track nonces separately for each account - we may have >1 accounts pending at some point in time.
But we don't need a full bitmap; just a strictly increasing "highest nonce used so far".
| const SDK_REPO = "https://github.com/Agoric/agoric-sdk.git"; | ||
| const SDK_DIR = "/usr/src/agoric-sdk-cp"; | ||
| const BRANCH_NAME = "master"; | ||
| const BRANCH_NAME = "rs-use-nonce-in-axelar-gmp-contract"; |
There was a problem hiding this comment.
is this PR supposed to land with this change?
There was a problem hiding this comment.
relay.ts is meant to be a super generalized script. please create a new one instead for similar to relayWithTokens.ts etc
| using AddressToString for address; | ||
|
|
||
| address _gateway; | ||
| address gatewayAddr; |
There was a problem hiding this comment.
the _ indicates that it is a private variable. would be good to add that to the new name as well
closes:
refs:
The PR does the following:
chainNameFactorykeccak256constantusedNoncesmapping to prevent replay attacksnoncevalue